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.

Sign up for GitHub

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

Jump to bottom

Comparison of polynomials from different rings throws inconsistently #4107

Open
lkastner opened this issue Sep 18, 2024 · 5 comments · May be fixed by Nemocas/AbstractAlgebra.jl#1800, Nemocas/AbstractAlgebra.jl#1853 or Nemocas/AbstractAlgebra.jl#1854
Open

Comparison of polynomials from different rings throws inconsistently #4107

lkastner opened this issue Sep 18, 2024 · 5 comments · May be fixed by Nemocas/AbstractAlgebra.jl#1800, Nemocas/AbstractAlgebra.jl#1853 or Nemocas/AbstractAlgebra.jl#1854
Labels
bug Something isn't working

Comments

@lkastner
Copy link
Member

Describe the bug
When comparing polynomials from different rings I get inconsistent behaviour. Sometimes I just get false and sometimes an error.

To Reproduce

julia> R0,(x0) = polynomial_ring(QQ,1)
(Multivariate polynomial ring in 1 variable over QQ, QQMPolyRingElem[x1])

julia> R1,(x1) = polynomial_ring(ZZ,1)
(Multivariate polynomial ring in 1 variable over ZZ, ZZMPolyRingElem[x1])

julia> R2,(x2) = polynomial_ring(GF(2),1)
(Multivariate polynomial ring in 1 variable over GF(2), FqMPolyRingElem[x1])

julia> x0 == x1
false

julia> x0 == x2
false

julia> R3,(x3) = polynomial_ring(GF(3),1)
(Multivariate polynomial ring in 1 variable over GF(3), FqMPolyRingElem[x1])

julia> x2 == x3
ERROR: parents do not match
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] check_parent
   @ ~/.julia/packages/AbstractAlgebra/QjFtH/src/AbstractAlgebra.jl:208 [inlined]
 [3] check_parent
   @ ~/.julia/packages/AbstractAlgebra/QjFtH/src/AbstractAlgebra.jl:207 [inlined]
 [4] ==(a::FqMPolyRingElem, b::FqMPolyRingElem)
   @ Nemo ~/.julia/packages/Nemo/4P3uO/src/flint/fq_default_mpoly.jl:295
 [5] ==(A::Vector{FqMPolyRingElem}, B::Vector{FqMPolyRingElem})
   @ Base ./abstractarray.jl:2943
 [6] top-level scope
   @ REPL[18]:1

julia> x0 == x3
false

Expected behavior
We have many places where we first compare the "surrounding space" before comparing the actual objects. So I would expect an error to get thrown. E.g. try

cube(2) == cube(3)

Additional context
Maybe this needs a styleguide entry, I actually did not look it up.
@lkastner lkastner added the bug Something isn't working label Sep 18, 2024
@lgoettgens
Copy link
Member

Some background: In the cases with an error printed, the types of both arguments are equal, but the parents are not.
In the other cases, the types are not equal and thus julia delegated == to ===.

@joschmitt joschmitt linked a pull request Sep 19, 2024 that will close this issue
Open
@lgoettgens
Copy link
Member

Let's discuss the possible fix in Nemocas/AbstractAlgebra.jl#1800 in triage.

@mjrodgers
Copy link
Collaborator

It seems that there are cases where we would want an error rather than false when the parents are not ===. So maybe this inconsistency is hard to avoid. After discussing in triage, no conclusion on proper behavior was reached.

@mjrodgers
Copy link
Collaborator

arguably, always throwing an error might be preferable.

@fingolfin
Copy link
Member

In my personal experience (which of course may differ from what others experience) is that if I compare two polynomials from different rings, then this is always a user error (i.e., mine). In that situation a helpful error message that suggests what's wrong for me is preferable to getting a result that may confuse me (i.e. think of f and g both printing as x but f == g giving false and the user going "WTF").

That said, perhaps people have concrete examples in mind were it is helpful to be allowed to compared polynomials with different parents directly. In that case it'd be great to learn about them here.

@lgoettgens lgoettgens linked a pull request Oct 9, 2024 that will close this issue
Open
@fingolfin fingolfin linked a pull request Oct 9, 2024 that will close this issue
Draft
@fingolfin fingolfin removed the triage label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
4 participants
@fingolfin @lkastner @mjrodgers @lgoettgens