Nemocas / AbstractAlgebra.jl

Generic abstract algebra functionality in pure Julia (no C dependencies)
https://nemocas.github.io/AbstractAlgebra.jl/dev/index.html
Other
163 stars 63 forks source link

Remove MPolyRing(R::Ring, n::Int) #1728

Closed fingolfin closed 3 months ago

fingolfin commented 3 months ago

Analogue to PolyRing(), this one doesn't seem to be used at all

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.33%. Comparing base (65f9956) to head (39c56bb).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1728 +/- ## ======================================= Coverage 87.32% 87.33% ======================================= Files 117 117 Lines 29768 29767 -1 ======================================= Hits 25996 25996 + Misses 3772 3771 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fingolfin commented 3 months ago

HeckeCI failures look like https://github.com/thofma/Hecke.jl/issues/1526 i.e. unrelated to this PR

lgoettgens commented 3 months ago

I think this is technically a breaking change to Oscar. Do we care about that? If yes, this should be deprecated instead of removed. But in any case, the change should be made in a breaking release of AA.

fingolfin commented 3 months ago

MPolyRing is an undocumented function, and not even exported. I don't think we generally consider removing such a function a breaking change.

lgoettgens commented 3 months ago

I agree with the missing documentation, however it is indeed exported:

julia> using Oscar
  ___   ____   ____    _    ____
 / _ \ / ___| / ___|  / \  |  _ \   |  Combining ANTIC, GAP, Polymake, Singular
| | | |\___ \| |     / _ \ | |_) |  |  Type "?Oscar" for more information
| |_| | ___) | |___ / ___ \|  _ <   |  Manual: https://docs.oscar-system.org
 \___/ |____/ \____/_/   \_\_| \_\  |  Version 1.0.3

julia> MPolyRing(QQ, 2)
Multivariate polynomial ring in 2 variables x1, x2
  over rational field

julia> Base.isexported(Oscar, :MPolyRing)
true

julia> Base.isexported(Oscar.AbstractAlgebra, :MPolyRing)
true

I am not perse against this change, I definitely support all of this cleanup. But I wanted to mention things like this to make it a conscious decision and not some accidental result.