Implement algebra_containment from Singular (issue #34502) #36030

jhpalmieri · 2023-08-03T19:52:39Z

This implements subalgebra containment for polynomials: whether a polynomial is contained in the subalgebra determined by given generators.

This fixes #34502. It provides several algorithms for the calculution, using Singular's algebra_containment, Singular's inSubring, or Sage's Groebner basis tools.

📝 Checklist

The title is concise, informative, and self-explanatory.
The description explains in detail what this PR is about.
I have linked a relevant issue or discussion.
I have created tests covering the changes.
I have updated the documentation accordingly.

⌛ Dependencies

kwankyu · 2023-08-15T01:05:09Z

src/sage/rings/polynomial/multi_polynomial_libsingular.pyx

+ INPUT:
+
+ - ``J`` -- iterable of elements of the parent polynomial ring
+


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

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

kwankyu · 2023-08-15T01:06:02Z

src/sage/rings/polynomial/multi_polynomial_libsingular.pyx

+ https://www.singular.uni-kl.de/Manual/4-2-1/sing_1247.htm#SEC1328. The
+ Singular documentation suggests that this is frequently faster than the
+ next option.
+


Actually the documentation says inSubring is sometimes faster. No?

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

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.

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.

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.

kwankyu · 2023-08-15T01:09:38Z

src/sage/rings/polynomial/multi_polynomial_libsingular.pyx

+ - ``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


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.

Good idea, I'll do that.

kwankyu · 2023-08-15T04:33:18Z

src/sage/rings/polynomial/multi_polynomial_libsingular.pyx

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


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

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

Or see the comment below.

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.

kwankyu · 2023-08-15T04:39:19Z

src/sage/rings/polynomial/multi_polynomial_libsingular.pyx

@@ -5757,6 +5757,83 @@ cdef class MPolynomial_libsingular(MPolynomial_libsingular_base):
 else:
 return self * self.denominator()

+ def in_subalgebra(self, J, algorithm="algebra_containment"):
+ """


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.

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.

kwankyu · 2023-08-15T05:35:20Z

src/sage/rings/polynomial/multi_polynomial_libsingular.pyx

+ ideal. The polynomial is contained in the subalgebra if
+ and only if the remainder involves only the new variables
+ `z_i`.
+


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.

Done. The built documentation looks okay to me.

github-actions · 2023-08-15T21:04:23Z

Documentation preview for this PR (built with commit 851126b; changes) is ready! 🎉

kwankyu

Thanks. LGTM.

jhpalmieri · 2023-08-16T01:39:13Z

Thanks. LGTM.

Thanks for reviewing and for all of your feedback!

…agemath#34502)  This implements subalgebra containment for polynomials: whether a polynomial is contained in the subalgebra determined by given generators.     This fixes sagemath#34502. It provides several algorithms for the calculution, using Singular's algebra_containment, Singular's inSubring, or Sage's Groebner basis tools. ### 📝 Checklist     - [X] The title is concise, informative, and self-explanatory. - [X] The description explains in detail what this PR is about. - [X] I have linked a relevant issue or discussion. - [X] I have created tests covering the changes. - [X] I have updated the documentation accordingly. ### ⌛ Dependencies   URL: sagemath#36030 Reported by: John H. Palmieri Reviewer(s): John H. Palmieri, Kwankyu Lee

Implement algebra_containment from Singular (issue sagemath#34502)

729316e

jhpalmieri force-pushed the algebra-containment branch from 301dfad to 729316e Compare August 3, 2023 20:09

jhpalmieri added t: enhancement p: minor / 4 s: needs review c: algebra labels Aug 3, 2023

kwankyu reviewed Aug 15, 2023

View reviewed changes

jhpalmieri added 2 commits August 14, 2023 20:04

Merge branch 'develop' into algebra-containment

4f5e093

pr sagemath#36030: silence some Singular messages

c7bb2cf

kwankyu reviewed Aug 15, 2023

View reviewed changes

pr sagemath#36030: fix docstring

3f0a49e

kwankyu reviewed Aug 15, 2023

View reviewed changes

pr sagemath#36030: fix docstring markup for quoted strings

851126b

kwankyu approved these changes Aug 15, 2023

View reviewed changes

kwankyu added s: positive review and removed s: needs review labels Aug 15, 2023

vbraun merged commit 811ac59 into sagemath:develop Aug 27, 2023
13 of 14 checks passed

github-actions bot removed the s: positive review label Aug 27, 2023

mkoeppe added this to the sage-10.2 milestone Aug 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement algebra_containment from Singular (issue #34502) #36030

Implement algebra_containment from Singular (issue #34502) #36030

jhpalmieri commented Aug 3, 2023

kwankyu Aug 15, 2023

jhpalmieri Aug 15, 2023

kwankyu Aug 15, 2023

kwankyu Aug 15, 2023

jhpalmieri Aug 15, 2023

jhpalmieri Aug 15, 2023

kwankyu Aug 15, 2023

kwankyu Aug 15, 2023

jhpalmieri Aug 15, 2023

kwankyu Aug 15, 2023

kwankyu Aug 15, 2023 •

edited

Loading

kwankyu Aug 15, 2023 •

edited

Loading

jhpalmieri Aug 15, 2023

kwankyu Aug 15, 2023 •

edited

Loading

jhpalmieri Aug 15, 2023

kwankyu Aug 15, 2023

kwankyu Aug 15, 2023 •

edited

Loading

jhpalmieri Aug 15, 2023

github-actions bot commented Aug 15, 2023

kwankyu left a comment

jhpalmieri commented Aug 16, 2023

		INPUT:

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


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

Implement algebra_containment from Singular (issue #34502) #36030

Implement algebra_containment from Singular (issue #34502) #36030

Conversation

jhpalmieri commented Aug 3, 2023

📝 Checklist

⌛ Dependencies

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwankyu Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

kwankyu Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwankyu Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwankyu Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 15, 2023

kwankyu left a comment

Choose a reason for hiding this comment

jhpalmieri commented Aug 16, 2023

kwankyu Aug 15, 2023 •

edited

Loading

kwankyu Aug 15, 2023 •

edited

Loading

kwankyu Aug 15, 2023 •

edited

Loading

kwankyu Aug 15, 2023 •

edited

Loading