Missing hash methods

[fingolfin]

There are a bunch of our types which implement == but not hash, which leads to confusing behavior and bugs. E.g. for a vector consisting of two such objects which are equal but not identical, the function unique will return a vector with both objects, instead of just one. @YueRen just re-discovered this for Polyhedron, but there are more affected types.

Perhaps someone could write a little script to find more instances semi-automatically (e.g.: iterate over all objects in the Oscar module; for each which is a type, check if there is a custom Base.== method defined; and then for each check if also a custom Base.hash method is defined; if not, print the type).

In the meantime, here is a manual list which is certainly incomplete (and I also might have included some things incorrectly, because I overlooked a hash method). Feel free to edit this list or request changes.

• Cone
• PolyhedralFan (doesn't implement ==)
• Polyhedron
• ToricDivisor (see ToricVarieties: Add Base.hash #1264)
• ...

Also for a few types we should perhaps improve the existing hash methods. E.g. we have this right now:

Base.hash(x::GAPGroup, h::UInt) = h # FIXME
Base.hash(x::GAPGroupElem, h::UInt) = h # FIXME

I think they should at least produce different hashes according to their type. Thus a slightly better approach might be to do something like this:

Base.hash(x::T, h::UInt) where {T <: Union{GAPGroup, GAPGroupElem} = hash(T, h)

But we could still do more, e.g. for permutation groups the degree could be included, and arguably also the order (I mean, computing the order costs us, but if we compute a hash for a group, then we probably will also test equality at some point, and then we need a stabilizer chain anyway, so whatever).

[lgoettgens]

I wrote some small script (NEEDS julia 1.11.1!!!!) to get all cases, including the package that defines the type, and the file and line number the == method is defined (in some nice printing):

list = map(
  filter(names(Oscar; all=true)) do name
    isdefined(Oscar, name) || return false            # remove all wrong exports (see #1964)
    T = getfield(Oscar, name)
    T isa DataType || T isa UnionAll || return false  # remove all non-types and non-parametric-types
    parentmodule(==, (T, T)) == Base && return false  # remove everything without custom ==
    loc = functionloc(==, (T, T))
    endswith(loc[1], "julia/base/Base.jl") && loc[2] == 207 && return false  # remove everything without custom ==
    T <: AbstractArray && endswith(loc[1], "julia/base/abstractarray.jl") && loc[2] == 3028 && return false  # remove AbstractArray subtypes as Base provides both == and hash for them
    parentmodule(hash, (T, UInt)) == Base             # keep iff there is no custom hash
  end,
) do name
  T = getfield(Oscar, name)
  loc = functionloc(==, (T, T))
  if occursin("Oscar.jl/", loc[1])
    loc_cleaned = loc[1][first(findlast("Oscar.jl/", loc[1])):end]
  elseif occursin("packages/", loc[1])
    loc_cleaned = loc[1][(first(findlast("packages/", loc[1])) + length("packages/")):end]
  elseif occursin("julia/base/", loc[1])
    loc_cleaned = loc[1][first(findlast("julia/base/", loc[1])):end]
  else
    loc_cleaned = loc[1]
  end
  name, parentmodule(T), (loc_cleaned, loc[2])
end;

Currently (on cd79a0b), I get the following list of things:

julia> show(IOContext(stdout, :limit => false), "text/plain", list)
6-element Vector{Tuple{Symbol, Module, Tuple{String, Int32}}}:
 (:ClassField, Hecke, ("Hecke/3LoSB/src/RCF/class_fields.jl", 240))
 (:FinGenAbGroupHom, Hecke, ("Hecke/3LoSB/src/GrpAb/Map.jl", 217))
 (:FractionalIdeal, Oscar, ("Oscar.jl/src/Rings/FractionalIdeal.jl", 55))
 (:GenOrdFracIdl, Hecke, ("Hecke/3LoSB/src/GenOrd/FractionalIdeal.jl", 272))
 (:OrdLocElem, Hecke, ("AbstractAlgebra/lVsyJ/src/NCRings.jl", 86))
 (:SLPoly, Oscar, ("AbstractAlgebra/lVsyJ/src/NCRings.jl", 86))

One needs unfortunately to check the == methods by hand. If they are just delegating to objectid(-) == objected(-), I would propose to remove them, to make disappear from this list.

[HechtiDerLachs]

@HechtiDerLachs will look into the Geometry

Done, see #4305 .

[lgoettgens]

I updated the list above.

@HechtiDerLachs It seems you missed some types in AlgebraicGeometry/Schemes/Sheaves/IdealSheaves.jl (see the list above for the concrete types)

[lgoettgens]

With #4366 and #4369 all (or maybe I missed some very few) offenders in Oscar should be addressed. I'll update the list once they are merged.

[alexej-jordan]

#4354 has just been merged, too

[lgoettgens]

Currently (on cd79a0b), I get the following list of things:

julia> show(IOContext(stdout, :limit => false), "text/plain", list)
6-element Vector{Tuple{Symbol, Module, Tuple{String, Int32}}}:
 (:ClassField, Hecke, ("Hecke/3LoSB/src/RCF/class_fields.jl", 240))
 (:FinGenAbGroupHom, Hecke, ("Hecke/3LoSB/src/GrpAb/Map.jl", 217))
 (:FractionalIdeal, Oscar, ("Oscar.jl/src/Rings/FractionalIdeal.jl", 55))
 (:GenOrdFracIdl, Hecke, ("Hecke/3LoSB/src/GenOrd/FractionalIdeal.jl", 272))
 (:OrdLocElem, Hecke, ("AbstractAlgebra/lVsyJ/src/NCRings.jl", 86))
 (:SLPoly, Oscar, ("AbstractAlgebra/lVsyJ/src/NCRings.jl", 86))

After new releases of Hecke and AA are now available here, the list is now drastically shorter.