-
-
Notifications
You must be signed in to change notification settings - Fork 487
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
let the category setup handle the ideals #38821
Conversation
Documentation preview for this PR (built with commit e423a38; changes) is ready! 🎉 |
note : the removal of "ideal" in multi-polynomial requires a careful check, as it may have hidden side-effects |
At first sight, it seems to be ok thanks to the |
Co-authored-by: Martin Rubey <axiomize@yahoo.de>
Co-authored-by: Martin Rubey <axiomize@yahoo.de>
# note: we set g = gcd(g, g) to "canonicalize" the generator: | ||
# make polynomials monic, etc. | ||
g = g.gcd(g) | ||
except (AttributeError, NotImplementedError, IndexError): |
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.
It would be good to have a test for this line, in particular because IndexError
was added.
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.
so far, I was not able to find a ring that pass through that case.
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.
https://en.wikipedia.org/wiki/Euclidean_domain has
sage: R.<x,y> = PolynomialRing(QQ)
sage: Q = R.quotient(x^2+y^2+1)
sage: Q in PrincipalIdealDomains()
False
Also, if I made no mistake,
sage: R.<x> = QQ[]
sage: K.<a> = QQ.extension(x^2 + 19); K
Number Field in a with defining polynomial x^2 + 19
sage: K
Number Field in a with defining polynomial x^2 + 19
sage: K in PrincipalIdealDomains()
True
sage: K in EuclideanDomains()
True
(Disclaimer: I know nothing about this kind of math)
So, I guess it's best to leave it in.
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.
One can declare the category a posteriori
Q._refine_category_(PrincipalIdealDomains())
but then creating the ideal follows a specific code-path for quotient rings.
For the second part, I guess you mean the ring of integers rather than the number field. I tried, and there is also a specific code-path for ideals there.
I played a little with the patch and it looks good to me. I found a bug, but that's present in develop, too:
|
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.
LGTM
thanks ! So positive review ? |
sorry, forgot |
as another step towards using `Parent` everywhere ### 📝 Checklist - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. Dependencies: sagemath#38821 URL: sagemath#38881 Reported by: Frédéric Chapoton Reviewer(s): Martin Rubey
This is removing the
ideal
methods in the oldRing
andField
classes, moving them to the category setup.Also removing one custom
ideal
method in multiple polynomial rings.📝 Checklist