-
Notifications
You must be signed in to change notification settings - Fork 71
Add more syntactic sugar for polynomial ring constructors #2191
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2191 +/- ##
==========================================
+ Coverage 87.95% 87.99% +0.04%
==========================================
Files 127 127
Lines 31791 31772 -19
==========================================
- Hits 27961 27958 -3
+ Misses 3830 3814 -16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Every time I see this I am surprised that we have this syntax. I think this change here makes it actually useful. |
|
I would simply disallow Otherwise: this change is breaking, but I think it is OK for OSCAR because this feature is (a) not well-known or widely used, and (b) primarily intended for REPL use / demos anyway. So some notebooks somewhere might be broken by it, but so be it. (My only concern would be the OSCAR booktests... I don't know if any use this feature???) |
|
Grepping for |
|
I approved this as this is fine from my POV; but since it is breaking we will only merge this shortly before a release. In the meantime, one more request: it would be good to document this feature. Specifically the text at https://nemocas.github.io/AbstractAlgebra.jl/dev/mpolynomial/#Polynomial-ring-constructors should be extended to explain it and show it in an example. |
|
ping @lgoettgens |
lgoettgens
left a comment
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 a purely technical POV, the changes here look fine.
While reading a question by @lkastner on Slack I saw this polynomial ring construction:
And I thought that this should be a little more convenient like this:
QQ[:x][:y]existed before this PR but it returned only the polynomial ring inyand the variableyitself. I've extended this idea to support all combinations of nested univariate and multivariate polynomial rings. While at it, I also noticed that we allowR[:x,:y]for non-commutative ringsReven though there is no implementation of multivariate polynomial rings over non-commutative rings. (One can argue what this should even mean, I'd rather expectR[:x,:y]to be the free algebra onR.) Simply removing this method would causeQQ[:x]to create a multivariate polynomial ring in just one variable. So, I made the multivariate shorthand construction more explicit, such that at least two variables are needed for multivariate polynomial rings. If desired I can separate this change into another PR.