Add more syntactic sugar for polynomial ring constructors
While reading a question by @lkastner on Slack I saw this polynomial ring construction:
julia> K = algebraic_closure(QQ)
Algebraic closure of rational field
julia> Kx, xx = K[:x]
(Univariate polynomial ring in x over QQBar, x)
julia> Kxy, yy = Kx[:y]
(Univariate polynomial ring in y over Kx, y)
And I thought that this should be a little more convenient like this:
julia> R, (x,y) = QQ[:x][:y]
(Univariate polynomial ring in y over univariate polynomial ring, AbstractAlgebra.Generic.Poly{AbstractAlgebra.Generic.Poly{Rational{BigInt}}}[x, y])
QQ[:x][:y] existed before this PR but it returned only the polynomial ring in y and the variable y itself. I've extended this idea to support all combinations of nested univariate and multivariate polynomial rings. While at it, I also noticed that we allow R[:x,:y] for non-commutative rings R even though there is no implementation of multivariate polynomial rings over non-commutative rings. (One can argue what this should even mean, I'd rather expect R[:x,:y] to be the free algebra on $x$ and $y$ over R.) Simply removing this method would cause QQ[: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.
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 87.99%. Comparing base (2980528) to head (1edb4bb).
:warning: Report is 6 commits behind head on master.
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
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
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 R[:x,:y] if R is not commutative / not a Ring. As Martin just tells me, this is what this PR does, so: good.
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 \]\[ in Oscar's test/book/ dir reveals no relevant hits, so I am rather confident this is fine there as well.
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