sage.rings: Reformat doctests, add # optional annotations#35457
sage.rings: Reformat doctests, add # optional annotations#35457vbraun merged 52 commits intosagemath:developfrom
sage.rings: Reformat doctests, add # optional annotations#35457Conversation
SageMath version 10.0.beta9, Release Date: 2023-04-13
| 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.
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.
Yes, it uses sage.groups.generic; but maybe I should ship this module with sagemath-categories
| 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.
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.
On the other hand, the sage.libs.singular gives attribution of this feature to a library...
There was a problem hiding this comment.
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.
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.
I am confused. Don't we need singular just to create P or I?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
@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.
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.
There was a problem hiding this comment.
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.
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,...padicsetc.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.ringsin 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
# optionaltags, breakingsage:lines) were made using the following Emacs macros.