Skip to content

Conversation

@SoongNoonien
Copy link
Member

In Nemocas/AbstractAlgebra.jl#2182 @fingolfin suggested to change the name of the field name mpoly_ring for universal polynomial rings. This causes a test failure in the action polynomial code. Previously the code directly accessed the field mpoly_ring which was a bad idea in the first place. I've now adapted it to call the new base_ring method for universal polynomial rings instead. Of course this will break the tests here again since Nemocas/AbstractAlgebra.jl#2182 isn't merged yet. Instead of base_ring(upr) we could write something like parent(data(upr())) which would serve the same purpose but be compatible with the current AbstractAlgebra release.

@SirToby25 Do you actually need these constructors which currently access the mpoly_ring field directly? Note that the current implementations is also rather inefficient.

@joschmitt joschmitt added experimental Only changes experimental parts of the code release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Oct 23, 2025
Comment on lines 109 to 113
function DifferencePolyRingElem{T}(dpr::DifferencePolyRing{T}, mpre::MPolyRingElem{T}) where {T}
upr = dpr.upoly_ring
@req upr.mpoly_ring === parent(mpre) "The parent does not match"
@req base_ring(upr) === parent(mpre) "The parent does not match"
new{T}(upr(collect(coefficients(mpre)), collect(exponents(mpre))), dpr, false, zeros(Int, length(mpre)))
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function DifferencePolyRingElem{T}(dpr::DifferencePolyRing{T}, mpre::MPolyRingElem{T}) where {T}
upr = dpr.upoly_ring
@req upr.mpoly_ring === parent(mpre) "The parent does not match"
@req base_ring(upr) === parent(mpre) "The parent does not match"
new{T}(upr(collect(coefficients(mpre)), collect(exponents(mpre))), dpr, false, zeros(Int, length(mpre)))
end

It seemed consequent at some point to also have this constructor, but I never use it nor is it realistic to expect a user to try this. From my PoV it can be removed completely.

Note that the corresponding tests in lines 649, 650, 655, 656 of experimental/ActionPolyRing/test/ActionPolyRing.jl need to be removed as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK then it's probably easiest to delete this for now, and we can add it back if and when it is needed, by that time we may have the new UniversalRing in place

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I can take care of that. I'll make a new PR which removes both constructors and their tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

experimental Only changes experimental parts of the code release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants