Skip to content
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

Relax signature of ==(::MPolyIdeal, ::MPolyIdeal) #2937

Merged
merged 1 commit into from
Oct 19, 2023

Conversation

joschmitt
Copy link
Member

Small change, but requires a long explanation:
The current signature of == for ideals in multivariate polynomial rings is

==(::MPolyIdeal{T}, ::MPolyIdeal{T}) where T

so this function is not called if one tries to compare ideals in polynomial rings of different type (T), instead the generic == is called:

julia> R, (x, y) = QQ["x", "y"];

julia> S, (u, v) = grade(R, [ 1, 1 ]);

julia> ideal(R, [x]) == ideal(S, [u])
false

julia> @which ideal(R, [x]) == ideal(S, [u])
==(x, y)
     @ Base Base.jl:127

The answer false is of course not wrong, but in my opinion this should error because the comparison doesn't make sense; this is what I added in this PR:

julia> ideal(R, [x]) == ideal(S, [u])
ERROR: Base rings must coincide.
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] check_base_rings(I::MPolyIdeal{QQMPolyRingElem}, J::MPolyIdeal{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}})
   @ Oscar ~/.julia/dev/Oscar/src/Rings/mpoly-ideals.jl:75
 [3] ==(I::MPolyIdeal{QQMPolyRingElem}, J::MPolyIdeal{MPolyDecRingElem{QQFieldElem, QQMPolyRingElem}})
   @ Oscar ~/.julia/dev/Oscar/src/Rings/mpoly-ideals.jl:993
 [4] top-level scope
   @ REPL[6]:1

julia> @which ideal(R, [x]) == ideal(S, [u])
==(I::MPolyIdeal, J::MPolyIdeal)
     @ Oscar ~/.julia/dev/Oscar/src/Rings/mpoly-ideals.jl:992

@joschmitt
Copy link
Member Author

I also added a check_base_rings to is_subset(::MPolyIdeal, ::MPolyIdeal); this function would work for rings which "look the same" but aren't (which is quite convenient but also leads to more problems than it solves in my opinion).

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thank you, good idea!

@fingolfin fingolfin enabled auto-merge (squash) October 19, 2023 14:03
@fingolfin fingolfin merged commit 2c7e3a4 into oscar-system:master Oct 19, 2023
9 of 12 checks passed
@joschmitt joschmitt deleted the js/ideal branch October 20, 2023 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants