AbstractAlgebra.jl icon indicating copy to clipboard operation
AbstractAlgebra.jl copied to clipboard

Add more syntactic sugar for polynomial ring constructors

Open SoongNoonien opened this issue 2 months ago • 6 comments

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.

SoongNoonien avatar Oct 21 '25 13:10 SoongNoonien

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.

codecov[bot] avatar Oct 21 '25 13:10 codecov[bot]

Every time I see this I am surprised that we have this syntax. I think this change here makes it actually useful.

thofma avatar Oct 22 '25 06:10 thofma

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???)

fingolfin avatar Oct 22 '25 12:10 fingolfin

Grepping for \]\[ in Oscar's test/book/ dir reveals no relevant hits, so I am rather confident this is fine there as well.

fingolfin avatar Oct 22 '25 12:10 fingolfin

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.

fingolfin avatar Oct 28 '25 23:10 fingolfin

ping @lgoettgens

fingolfin avatar Nov 05 '25 10:11 fingolfin