oscar-system / Oscar.jl

A comprehensive open source computer algebra system for computations in algebra, geometry, and number theory.
https://www.oscar-system.org
Other
345 stars 126 forks source link

Hashing is wrongly implemented for various ideals #4143

Open HechtiDerLachs opened 1 month ago

HechtiDerLachs commented 1 month ago

Try the following:

R, (x, y) = QQ[:x, :y]
I1 = ideal(R, x)
I2 = ideal(R, y)
I1 == I2 # returns true as it should
hash(I1) == hash(I2) # returns false!

According to the convention for hashes, objects which are == must also have the same hash. The failure of this probably makes unique! in lists of ideals fail:

length(unique!([I1, I2])) # result is 2, not 1!

Edit: I'm not sure this can be resolved in general. For ideals over polynomial rings we could hash a standard basis for the default_ordering of the base_ring. But for other rings like the localizations?

lgoettgens commented 1 month ago

x-ref https://github.com/oscar-system/Oscar.jl/issues/2222

fingolfin commented 1 month ago

There is simply no hash method for these ideals, hence the generic one from Base is used, which only is valid of == is implemented via ===.

So "someone" needs to define better (and fast!) hash methods for MPolyIdeal. I can think of three options:

  1. let hash just return zero. Of course then using these as dictionary keys becomes atrocious.
  2. let hash raise an error
  3. compute a hash over some canonical basis or so -- but computing those is expensive; even if we cache them I am not sure this is desirable??
thofma commented 1 month ago

The problem is that there are methods in Base which use hashing without explicitly documenting it. Since this is an implementation detail, there might even be methods which start using hashing / dictionaries in the future. The most prominent culprit is unique/unique!.

I don't fully understand the "atrocious" part. For example, implementing unique(x) without hashing will have the same number of == (or isequal) as an approach via a dictionary where all elements hash to zero.

HechtiDerLachs commented 1 month ago

Just as an aside: Implementing a dummy hash for multivariate polynomial ideals in #4142 did not yet seem to remove the failure of unique for me. But other than that, I agree with @thofma .

joschmitt commented 1 month ago

I'm late to the party, but when I put the ideals from your example in OSCAR I get false which seems correct to me.

julia> R, (x, y) = QQ[:x, :y]
(Multivariate polynomial ring in 2 variables over QQ, QQMPolyRingElem[x, y])

julia> I1 = ideal(R, x)
Ideal generated by
  x

julia> I2 = ideal(R, y)
Ideal generated by
  y

julia> I1 == I2
false

Did you mean to do some kind of quotient construction?