Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement algebra_containment from Singular (issue #34502) #36030

Merged
merged 5 commits into from
Aug 27, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 72 additions & 0 deletions src/sage/rings/polynomial/multi_polynomial_libsingular.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -5744,6 +5744,78 @@ cdef class MPolynomial_libsingular(MPolynomial_libsingular_base):
else:
return self * self.denominator()

def in_subalgebra(self, J, algorithm=None):
"""
Copy link
Collaborator

@kwankyu kwankyu Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usual idiom is actually to write

def in_subalgebra(self, J, algorithm=None):
   ...
   if algorithm is None:
       algorithm = "algebra_containment"
   else:
       algorithm = algorithm.lower()

This is just a suggestion. You may keep your current code. In this case, see the comment above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with this (to me) is that the signature shows None as the default value, in which case I feel like I should explain it in the docstring. It may be the convention to ignore this, but I worry that it will confuse some user.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

Return whether this polynomial is contained in the subalgebra
generated by ``J``

INPUT:

- ``J`` -- iterable of elements of the parent polynomial ring

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"iterable of elememts of ..." is precise, but I think just "elements of ..." is simple and easy. Just a suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it to "list of elements ..."

- ``algorithm`` -- can be ``None`` (the default), "algebra_containment",
"inSubring", or "groebner".

Copy link
Collaborator

@kwankyu kwankyu Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you removed None from the code below, this docstring should be updated, like

        - ``algorithm`` -- one of  "algebra_containment" (default),
          "inSubring", or "groebner".

Copy link
Collaborator

@kwankyu kwankyu Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or see the comment below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that you removed None from the code below, this docstring should be updated, like

        - ``algorithm`` -- one of  "algebra_containment" (default),
           "inSubring", or "groebner".

Fixed.

- ``None`` (the default): use "algebra_containment".

- "algebra_containment": use Singular's ``algebra_containment`` function,
https://www.singular.uni-kl.de/Manual/4-2-1/sing_1247.htm#SEC1328. The
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just

- ``'algebra_containment'``: (default) uses Singular's ``algebra_containment`` function ...

Then you can get rid of `None``, which is really not an option.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I'll do that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

Singular documentation suggests that this is frequently faster than the
next option.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the documentation says inSubring is sometimes faster. No?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I don't like "algebra_containment" as the default algorithm, since it outputs cluttering logs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the documentation says inSubring is sometimes faster. No?

The documentation for inSubring says, "the proc algebra_containment tests the same using a different algorithm, which is often faster". The documentation for algebra_containment says, "The proc inSubring uses a different algorithm which is sometimes faster." I interpret "often faster" for algebra_containment as better than "sometimes faster" for inSubring, so I think we should prefer algebra_containment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I don't like "algebra_containment" as the default algorithm, since it outputs cluttering logs.

By copying some code from function_field/function_field_polymod.py, I think I have managed to silence the logs.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the documentation says inSubring is sometimes faster. No?

The documentation for inSubring says, "the proc algebra_containment tests the same using a different algorithm, which is often faster". The documentation for algebra_containment says, "The proc inSubring uses a different algorithm which is sometimes faster." I interpret "often faster" for algebra_containment as better than "sometimes faster" for inSubring, so I think we should prefer algebra_containment.

OK.

Also I don't like "algebra_containment" as the default algorithm, since it outputs cluttering logs.

By copying some code from function_field/function_field_polymod.py, I think I have managed to silence the logs.

Nice! I was thinking of that too. Thanks.

- "inSubring": use Singular's ``inSubring`` function,
https://www.singular.uni-kl.de/Manual/4-2-0/sing_1240.htm#SEC1321.

- "groebner": use the algorithm described in Singular's
documentation, but within Sage: if the subalgebra
generators are `y_1`, ..., `y_m`, then create a new
polynomial algebra with the old generators along with new
ones: `z_1`, ..., `z_m`. Create the ideal `(z_1 - y_1,
..., z_m - y_m)`, and reduce the polynomial modulo this
ideal. The polynomial is contained in the subalgebra if
and only if the remainder involves only the new variables
`z_i`.

Copy link
Collaborator

@kwankyu kwankyu Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matthias is massively correcting string literals as in this:

    def set_random_number_generator(self, rng='default'):
        """
-       Set the gsl random number generator to be one of ``default``,
-       ``luxury``, or ``taus``.
+       Set the gsl random number generator to be one of ``'default'``,
+       ``'luxury'``, or ``'taus'``.

        EXAMPLES::

So how about changing "groebner" (and others) to

``'groebner'`` 

? This would save him from some work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. The built documentation looks okay to me.

Note: the default algorithm may print some intermediate information.

EXAMPLES::

sage: P.<x,y,z> = QQ[]
sage: J = [x^2 + y^2, x^2 + z^2]
sage: (y^2).in_subalgebra(J)
...
False
sage: a = (x^2 + y^2) * (x^2 + z^2)
sage: a.in_subalgebra(J, algorithm='inSubring')
True
sage: (a^2).in_subalgebra(J, algorithm='groebner')
True
sage: (a + a^2).in_subalgebra(J)
...
True
"""
R = self.parent()
if algorithm is not None:
algorithm = algorithm.lower()
from sage.libs.singular.function import singular_function, lib as singular_lib
singular_lib('algebra.lib')
if algorithm is None or algorithm == "algebra_containment":
return singular_function('algebra_containment')(self, R.ideal(J)) == 1
elif algorithm == "insubring":
return singular_function('inSubring')(self, R.ideal(J))[0] == 1
elif algorithm == "groebner":
new_gens = [f"newgens{i}" for i in range(len(J))]
ngens = len(new_gens)
from sage.rings.polynomial.polynomial_ring_constructor import PolynomialRing
S = PolynomialRing(R.base_ring(), R.gens() + tuple(new_gens),
order=f'degrevlex({len(R.gens())}),degrevlex({ngens})')
new_gens = S.gens()[-ngens:]
I = S.ideal([g - S(j) for (g,j) in zip(new_gens, J)])
z = S(self).reduce(I)
return set(z.variables()).issubset(new_gens)
else:
raise ValueError("unknown algorithm")


def unpickle_MPolynomial_libsingular(MPolynomialRing_libsingular R, d):
"""
Expand Down
Loading