Add missing hash methods for maps

[fingolfin]

We have this method in Oscar (I think @HechtiDerLachs added it?):

# Problems arise with comparison for non-trivial coefficient maps 
# in all constellations. This calls for a streamlined set of commands 
# to check whether coefficient maps exist and if so, what they are, 
# so that they can be compared. 
function Base.:(==)(
    f::Map{<:MPolyAnyRing, <:MPolyAnyRing},
    g::Map{<:MPolyAnyRing, <:MPolyAnyRing}
   )
...

However, many of our Map types don't implement hash -- but implementing
== for a type requires that also hash is implemented for correctness.

As a result, we currently get this:

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

julia> f1 = hom(R, R, [y,x]);

julia> f2 = hom(R, R, [y,x]);

julia> f1 == f2
true

julia> hash(f1)
0xb7ee188fb7693c7c

julia> hash(f2)
0x683deafb5f3b9ec7

This patch proposes a fix. Note that the TODO in the new hash function (which points out that the coefficient map could also be taken into account for computing the hash) is just about getting a better hash, it is not needed for correctness.

A similar issue is fixed in experimental/GModule/Cohomology.jl ; that code there should perhaps only apply to FPModuleHomomorphism but that's a slightly bigger change....

I guess I should add tests ...

[HechtiDerLachs]

Thanks for the catch.

I think, I once added something like _has_coefficient_map to check whether or not there is one. This can not be deduced from the abstract Map-type and the more specialized types differ too much in their signatures. But in principle, the implementation of _has_coefficient_map should only depend on the input's type.

I think with that, one can fill the gaps.

[HechtiDerLachs]

Suggested change

	# TODO: add in coefficient_map if available
	_has_coefficient_map(f) && (h = hash(coefficient_map(f), h))

Problem is: coefficient maps can be anything. In particular they can be mathematically the same, even though they have different data types in the computer and, hence, most probably also different hashes. So one could also think about restricting the hash and comparison functions to maps without coefficient maps and throw an error.

[fingolfin]

I saw _has_coefficient_map and did not add code like in your suggestion precisely because of the issue you point out.

I guess could compute the image of the generators under the coefficient map and hash that; but it is not clear to me this will work in general, and also the performance impact (this will cause a bunch of allocations!) is unclear.

Of course we could cache the hash (rhyming not intended ;-) ) to overcome that.

[fingolfin]

But note that this is strictly optional: if we just don't consider the coefficient maps for the hash, we still have something that is correct.

[HechtiDerLachs]

but it is not clear to me this will work in general

No, it won't. Take, for instance, the map on (QQ[i])[x, y] which is the identity on the variables, but complex conjugation on the coefficients.

[HechtiDerLachs]

if we just don't consider the coefficient maps for the hash, we still have something that is correct.

True. So hashing could (and probably should) ignore the coefficient map. The equality check, on the other hand, should throw an error in case there are coefficient maps present which are not ===. In that case I think, we simply can not write generic code due to the generality allowed for coefficient maps.

@thofma : Am I seeing this correct?

[thofma]

Yes, and yes to throwing an error.

(Maybe just do hash(f::Map, h::UInt) = UInt(0) and be done with it.)

[fingolfin]

Error: fine by me.

hash(f::Map, h::UInt) = UInt(0): this discards any previous hash results, which is bad when chaining hashes; better would be hash(f::Map, h::UInt) = h . That said, I personally prefer an error in this case, i.e. hash(f::Map, h::UInt) = error("hashing not implemented for this map type, please report this as bug to the OSCAR team") :-)

[fingolfin]

Would be great to get this approved and merged (I applied Lars' suggestion)

[lgoettgens]

I think the current state of this is reasonable. We can change it again in the future, if something comes up