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

Add missing hash methods for maps #3315

Merged
merged 2 commits into from
Feb 7, 2024
Merged

Add missing hash methods for maps #3315

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8152506
Add missing hash methods for maps
fingolfin Feb 5, 2024
f71b1e4
Apply suggestions from code review
fingolfin Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions experimental/GModule/Cohomology.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1713,9 +1713,15 @@ function Oscar.matrix(M::FreeModuleHom{FreeMod{QQAbElem}, FreeMod{QQAbElem}})
end

function ==(a::Union{Generic.ModuleHomomorphism, Generic.ModuleIsomorphism}, b::Union{Generic.ModuleHomomorphism, Generic.ModuleIsomorphism})
domain(a) === domain(b) || return false
codomain(a) === codomain(b) || return false
return matrix(a) == matrix(b)
end

function Base.hash(a::Union{Generic.ModuleHomomorphism, Generic.ModuleIsomorphism}, h::UInt)
return hash(matrix(a), h)
fingolfin marked this conversation as resolved.
Show resolved Hide resolved
end

function Oscar.id_hom(A::AbstractAlgebra.FPModule)
return Generic.ModuleHomomorphism(A, A, identity_matrix(base_ring(A), ngens(A)))
end
Expand Down
8 changes: 8 additions & 0 deletions src/Rings/mpolyquo-localizations.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2407,6 +2407,14 @@ function Base.:(==)(
return all(x->f(x) == g(x), gens(domain(f)))
end

function Base.hash(f::Map{<:MPolyAnyRing, <:MPolyAnyRing}, h::UInt)
h = hash(domain(f), h)
h = hash(codomain(f), h)
# TODO: add in coefficient_map if available
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, and yes to throwing an error.

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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") :-)

h = hash(f.(gens(domain(f))), h)
return h
end

coefficient_map(f::MPolyLocalizedRingHom) = coefficient_map(restricted_map(f))
coefficient_map(f::MPolyQuoLocalizedRingHom) = coefficient_map(restricted_map(f))

Expand Down
Loading