-
-
Notifications
You must be signed in to change notification settings - Fork 655
sage.rings
: Reformat doctests, add # optional
annotations
#35457
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
sage.rings
: Reformat doctests, add # optional
annotations
#35457
Conversation
SageMath version 10.0.beta9, Release Date: 2023-04-13
@@ -850,6 +850,7 @@ cdef class CachedFunction(): | |||
sage: I = P*[x,y] | |||
sage: from sage.misc.sageinspect import sage_getdoc | |||
sage: print(sage_getdoc(I.groebner_basis)) # indirect doctest | |||
WARNING: the enclosing module is marked... | |||
Return the reduced Groebner basis of this ideal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the warning message. Does it show in the doctesting?
[1, 15, 13, 3, 9, 7, 5, 11] | ||
sage: (mod(22,31)^200).nth_root(200) | ||
sage: (mod(22,31)^200).nth_root(200) # optional - sage.groups | ||
5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it uses sage.groups.generic
; but maybe I should ship this module with sagemath-categories
sage: I = P*[a, b] | ||
sage: a + b in I | ||
sage: I = P * [a, b] | ||
sage: a + b in I # optional - sage.libs.singular | ||
True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we have feature sage.rings.polynomial
instead of sage.libs.singular
? I feel uncomfortable with those features sage.libs.xxx
since the feature name reflects implementation details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, the sage.libs.singular
gives attribution of this feature to a library...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above code uses just polynomials (over rationals). So it is obvious that it needs sage.rings.polynomial
if such feature exists. But to think of sage.libs.singular
, the developer should be well aware of singular
is used to implement polynomials in sage. This is an implementation detail, and is not obvious for some developer who has worked only in, say, combinatorics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Here polynomials are already available, but to compute the inclusion in I
needs singular. That is is the point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused. Don't we need singular just to create P
or I
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sage has an implementation for multivariate polynomials using dictionaries. It falls back to it when Singular is not available. The ring and also ideals can be created without Singular. Only when Gröbner bases are needed, for example for ideal membership, Singular is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. So the existence of the tag # optional - sage.libs.singular
in the code is incomprehensible to those that are not familiar with how sage works internally with polynomials. I think this will become a maintenance burden later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a documentation issue that can be solved by expanding the docstring of the feature to explain what Sage uses the library for
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially we should say that singular is needed if your computation involves Groebner basis. But which computation involves Groebner basis is still not obvious to the uninitiated.
I am okay for now. The problem, if it turns to be real, may be solved later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, Sage has its own "toy" implementation of Groebner bases; but even that depends on Singular
Thanks very much for the review! |
SageMath version 10.0.rc0, Release Date: 2023-04-23
Easy merge. |
Test failures seem genuine. |
I agree. I think I have a fix for them already on another branch, I'll check |
From the linter:
This is apparently not from this PR, but this PR also touches the file. |
This is fixed by #35552, I think. |
Fixed |
pdf docs stil fail. Please do not set back to positive review until you have tested it |
Now it builds |
conflicts with #35562 though |
Test fail. Which part of "please do not set back to positive review until you have tested it" was unclear? |
"it". |
Documentation preview for this PR is ready! 🎉 |
sage: P.<e,d,c,b,a> = PolynomialRing(QQ,5,order='lex') | ||
sage: P.<e,d,c,b,a> = PolynomialRing(QQ, 5, order='lex'); P.rename("P") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkoeppe I don't know what's going on here but this rename doesn't seem to be supported:
$ sage
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 10.0, Release Date: 2023-05-20 │
│ Using Python 3.11.4. Type "help()" for help. │
└────────────────────────────────────────────────────────────────────┘
sage: P.<e,d,c,b,a> = PolynomialRing(QQ, 5, order='lex')
sage: P.rename("P")
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
File /usr/lib/python3.11/site-packages/sage/structure/sage_object.pyx:121, in sage.structure.sage_object.SageObject.rename (build/cythonized/sage/structure/sage_object.c:2154)()
120 try:
--> 121 self.__custom_name = str(x)
122 except AttributeError:
AttributeError: 'sage.rings.polynomial.multi_polynomial_libsingular.MPolynomialRing_libsingular' object has no attribute '__custom_name'
During handling of the above exception, another exception occurred:
NotImplementedError Traceback (most recent call last)
Cell In[2], line 1
----> 1 P.rename("P")
File /usr/lib/python3.11/site-packages/sage/structure/sage_object.pyx:123, in sage.structure.sage_object.SageObject.rename (build/cythonized/sage/structure/sage_object.c:2203)()
121 self.__custom_name = str(x)
122 except AttributeError:
--> 123 raise NotImplementedError("object does not support renaming: %s" % self)
124
125 def reset_name(self):
NotImplementedError: object does not support renaming: Multivariate Polynomial Ring in e, d, c, b, a over Rational Field
sage:
It's not working for me in 10.1.beta7 either. I don't know what is going on, since I had not seen this error before and this was merged in 10.1.beta1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #35749 already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #35749 already
Is that the right PR? That one is 157 commits and mentions nothing of the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment
https://github.com/sagemath/sage/pull/35749/files#r1257472203
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the top of the description of #35749:
- Fixing the handling of file-level # optional tags.
- Some files were not being doctested; fixing the recently introduced errors in doctests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I thought I was getting crazy. I was trying to test #35977 and getting this error I never saw before but now I could reproduce with old branches.
Now there are two more regressions in singular 4.3.2p3.
📚 Description
Adding doctest tags
# optional - sage.rings.finite_rings
,...number_field
,...padics
etc.While going through the doctests line by line, I also made the following changes:
The doctest tags are preparation for being able to test parts of
sage.rings
in a modularized setting, in which not all rings are available. See https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#doctest-only-dependenciesPart of:
📝 Checklist
⌛ Dependencies
The mass edits (adding
# optional
tags, breakingsage:
lines) were made using the following Emacs macros.