Add `hash` methods for many types with `==` method #2373

lgoettgens · 2023-05-10T14:48:21Z

This addresses parts of #2222 and resolves #1264.

Julia requires that equal elements (w.r.t. ==) hash to the same value to make Sets etc. work.
For many OSCAR types (cf. #2222), there currently is a dedicated ==, but no hash; thus contradicting the requirement.

codecov · 2023-05-11T12:38:57Z

Codecov Report

Merging #2373 (db87017) into master (53ea281) will decrease coverage by 0.13%.
The diff coverage is 23.52%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2373      +/-   ##
==========================================
- Coverage   72.94%   72.82%   -0.13%     
==========================================
  Files         405      405              
  Lines       53781    53874      +93     
==========================================
+ Hits        39232    39235       +3     
- Misses      14549    14639      +90

Impacted Files	Coverage Δ
...metry/ToricVarieties/ToricDivisors/constructors.jl	`60.41% <0.00%> (-7.03%)`	⬇️
...ry/ToricVarieties/ToricLineBundles/constructors.jl	`48.00% <0.00%> (-5.34%)`	⬇️
...etry/ToricVarieties/ToricMorphisms/constructors.jl	`60.00% <0.00%> (-6.67%)`	⬇️
src/Modules/ModulesGraded.jl	`55.13% <0.00%> (-0.26%)`	⬇️
src/PolyhedralGeometry/iterators.jl	`86.75% <0.00%> (-1.17%)`	⬇️
src/Rings/PBWAlgebra.jl	`94.20% <0.00%> (-0.52%)`	⬇️
src/Rings/mpoly-localizations.jl	`64.42% <0.00%> (-0.65%)`	⬇️
src/Rings/mpolyquo-localizations.jl	`66.70% <0.00%> (-0.60%)`	⬇️
src/TropicalGeometry/semiring.jl	`23.07% <0.00%> (-0.69%)`	⬇️
...try/ToricVarieties/AlgebraicCycles/constructors.jl	`53.68% <16.66%> (-2.99%)`	⬇️
... and 6 more

... and 3 files with indirect coverage changes

lgoettgens · 2023-05-11T14:31:07Z

Most implementations are purely based on the things that are compared in the == implementation right above. If x == y does something like issubset(x,y) || issubset(y,x) or iszero(reduce(x.arg - y.arg)) this is not used, and the hash will be the same for all elements with the same parent (or similar).

fieker

The hash functions apprea to be correct, but are very weak in many cases. Can someone from teh geometry crowed maybe have a look?
Fundamentally, I'm not sure if a weak hash function is better than throwing an error. Several hash functinos introduced here shuold never be used.

src/AlgebraicGeometry/ToricVarieties/AlgebraicCycles/constructors.jl

HechtiDerLachs · 2023-05-17T10:11:04Z

src/Rings/mpoly-localizations.jl

+function Base.hash(T::AbsMPolyMultSet, h::UInt)
+  b = 0x7ce51a28c47ec5e1 % UInt
+  h = hash(typeof(T), h)
+  h = hash(ambient_ring(T), h)
+  return xor(h, b)
+end


Thanks for the effort! However, this kind of hashing will not be sufficient. Instead, I propose to throw an error here which requires the user to overwrite the hash function for their concrete type.

Can I do that and push to this PR?

Do you mean something similar to this?

Oscar.jl/src/Rings/mpoly-localizations.jl

Line 529 in 96c2eed

error("comparison of multiplicative sets of type $(typeof(T)) and $(typeof(U)) is not implemented")

Yeah, that would be great. Please text me on slack to coordinate, so that we delete nothing by accidentally force pushing anything.

I added a warning in 0dfcbe8. Is this fine with you?

After looking a bit deeper into the concrete subtypes, I think there cannot be special implementations of hash for each type. There are many different implementations for issubset for different pairs of types, and equality is just defined generically using issubset. Since AbsMPolyMultSets of different concrete types can be equal, their hashes should coincide as well. And I do not see how to do that with specialized implementations.

src/Rings/mpoly-localizations.jl

src/Rings/mpolyquo-localizations.jl

fingolfin · 2023-05-19T14:52:13Z

In principle this is fine by me, but there is (a) a conflict and (b) this is marked as a draft, so I won't look closer for now.

lgoettgens · 2023-05-22T13:50:31Z

Let's see if the CI runs without any warnings.

fieker

function Base.hash(a::SubquoModuleElem, h::UInt)

the current hash does not depend on the element, only on the parent. This is julia-correct, but is this what you want? I'd rather throw an error... @HechtiDerLachs ?

Similar: GAPGroupHomomorphism @fingolfin is there a decent hash for f.map or should this be an error?

fingolfin · 2023-05-23T09:44:41Z

I don't see a way to define an efficient hash for group morphisms, so I think that should throw an error

lgoettgens · 2023-05-23T13:34:35Z

I would refrain from throwing errors in these cases to not render these types useless for anyone who needs to use them in some container/setting with hashing (even without knowning). Instead, I would like to have some kind of warning. If possible, this warning should be marking the CI as failed.
Is something like this possible?

thofma · 2023-05-23T17:31:27Z

If the warning is supposed to make CI fail, why not error in the first place?

Or why not define the hash to be 0 % UInt for the types where there is no efficient hashing? Then one can still use them for containers.

lgoettgens · 2023-05-23T17:34:04Z

If the warning is supposed to make CI fail, why not error in the first place?

Or why not define the hash to be 0 % UInt for the types where there is no efficient hashing? Then one can still use them for containers.

The latter is my proposal in this PR. I use every little bit of information to make at least some hashes different to at least circumvent hash conflicts for objects of completely different types.

fingolfin · 2023-05-24T22:28:09Z

Or why not define the hash to be 0 % UInt for the types where there is no efficient hashing? Then one can still use them for containers.

That's one way to put it, as an advantage. But there's also the counterpoint: with a 0-hash, one can use these objects as keys for a container, without noticing at first that this is a terrible idea with worst case performance. Other than for toy examples it won't be useful...

That's why I'd rather have an error in those cases...

fingolfin · 2023-06-07T10:24:54Z

src/Modules/UngradedModules.jl

+  b = 0x0361a2f6467e4200 % UInt
+  h = hash(parent(a), h)
+  return xor(h, b)


So this is (one of?) the contentious parts: we (@fieker and me) don't think this is really better (in practical applications) than a function returning a constant, and that of course is mostly useless. So we'd favor an error indicating "not implemented".

Actually, to make it stronger: we require this to merge the PR. However, of course if we run into actual problems caused by this, we can revisit the situation.

Changed in 722eccf.

I still think this is a bad idea and could potentially render Oscar useless for some users that rely on some julia Base functionality that uses hashing. I would prefer to return 0 or some constant and give a warning (that in the best case should make CI fail) instead. Then, some user can still use Oscar types for their use-case, although it may be very inefficient in those cases.

But if this is what you want, let's go with it.

src/Groups/homomorphisms.jl

fingolfin

Let's merge this if tests pass now

Talked with Claus it is OK

lgoettgens force-pushed the lg/hash branch 2 times, most recently from fa49b72 to 0f39d99 Compare May 11, 2023 10:10

lgoettgens changed the title ~~Add hash methods for types with == method~~ Add hash methods for many types with == method May 11, 2023

lgoettgens marked this pull request as ready for review May 11, 2023 14:31

lgoettgens force-pushed the lg/hash branch 2 times, most recently from 441a5a9 to 91bcf5d Compare May 11, 2023 14:57

fieker reviewed May 17, 2023

View reviewed changes

HereAround requested changes May 17, 2023

View reviewed changes

src/AlgebraicGeometry/ToricVarieties/AlgebraicCycles/constructors.jl Outdated Show resolved Hide resolved

lgoettgens force-pushed the lg/hash branch 2 times, most recently from 4bc4e96 to 2fc7c8e Compare May 17, 2023 09:45

HechtiDerLachs reviewed May 17, 2023

View reviewed changes

lgoettgens marked this pull request as draft May 17, 2023 10:40

HereAround approved these changes May 19, 2023

View reviewed changes

lgoettgens force-pushed the lg/hash branch from 7b1796c to a7d55ef Compare May 22, 2023 13:27

lgoettgens marked this pull request as ready for review May 22, 2023 13:36

lgoettgens marked this pull request as draft May 22, 2023 13:36

lgoettgens force-pushed the lg/hash branch from 7c336c3 to 0dfcbe8 Compare May 22, 2023 13:49

lgoettgens marked this pull request as ready for review May 22, 2023 13:50

fieker previously requested changes May 23, 2023

View reviewed changes

fingolfin reviewed Jun 7, 2023

View reviewed changes

src/Groups/homomorphisms.jl Show resolved Hide resolved

fingolfin approved these changes Jun 26, 2023

View reviewed changes

fingolfin closed this Jun 26, 2023

fingolfin reopened this Jun 26, 2023

lgoettgens added 23 commits June 26, 2023 10:43

Add hash(::GAPGroupHomomorphism)

b77284d

Add hash(::FreeMod_dec)

454a0e3

Add hash(::PBWAlgIdeal)

0f1130e

Add hash(::ModuleFPHom)

ade3c66

Add hash for `(Affine/Linear)(Halfspace/Hyperplane)

3250427

Add hash(::SesquilinearForm)

25577af

Add hash(::AbsMPolyMultSet)

5e9b5f3

Add hash(::CohomologyClass)

0467882

Add hash(::ElementOfGSet)

53962f3

Add hash(::MPolyLocalizedRingHom)

252984c

Add hash(::MPolyQuoLocalizedRingHom)

be883d1

Add hash(::RationalEquivalenceClass)

f007f4a

Enhance hash(::FreeMod) and hash(::AbstractFreeModElem)

8caa177

Add hash(::SubquoModuleElem)

5ae1f6e

Add hash(::ToricDivisor)

54b3eed

Add hash(::ToricDivisorClass)

e8a8ab5

Add hash(::ToricLineBundle)

5e05adc

Add hash(::ToricMorphism)

9d7b953

Add hash(::TropicalSemiringElem)

2c8bfd3

Update hash for MPolyQuoLocalizedRingHom and MPolyLocalizedRingHom

3b09a90

Update == and hash for toric types

14d6e1e

Let hash for AbsMPolyMultSet and SubquoModuleElem error

cb08ab6

Bump version (this PR is breaking)

db87017

lgoettgens force-pushed the lg/hash branch from 296aabf to db87017 Compare June 26, 2023 08:43

fingolfin merged commit 4f262f5 into oscar-system:master Jul 5, 2023

lgoettgens deleted the lg/hash branch July 5, 2023 10:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `hash` methods for many types with `==` method #2373

Add `hash` methods for many types with `==` method #2373

lgoettgens commented May 10, 2023 •

edited

Loading

codecov bot commented May 11, 2023 •

edited

Loading

lgoettgens commented May 11, 2023

fieker left a comment

HechtiDerLachs May 17, 2023

lgoettgens May 17, 2023 •

edited

Loading

lgoettgens May 22, 2023 •

edited

Loading

lgoettgens May 22, 2023

fingolfin commented May 19, 2023

lgoettgens commented May 22, 2023

fieker left a comment

fingolfin commented May 23, 2023

lgoettgens commented May 23, 2023

thofma commented May 23, 2023

lgoettgens commented May 23, 2023

fingolfin commented May 24, 2023

fingolfin Jun 7, 2023

lgoettgens Jun 7, 2023

fingolfin left a comment

Add hash methods for many types with == method #2373

Add hash methods for many types with == method #2373

Conversation

lgoettgens commented May 10, 2023 • edited Loading

codecov bot commented May 11, 2023 • edited Loading

Codecov Report

lgoettgens commented May 11, 2023

fieker left a comment

Choose a reason for hiding this comment

HechtiDerLachs May 17, 2023

Choose a reason for hiding this comment

lgoettgens May 17, 2023 • edited Loading

Choose a reason for hiding this comment

lgoettgens May 22, 2023 • edited Loading

Choose a reason for hiding this comment

lgoettgens May 22, 2023

Choose a reason for hiding this comment

fingolfin commented May 19, 2023

lgoettgens commented May 22, 2023

fieker left a comment

Choose a reason for hiding this comment

fingolfin commented May 23, 2023

lgoettgens commented May 23, 2023

thofma commented May 23, 2023

lgoettgens commented May 23, 2023

fingolfin commented May 24, 2023

fingolfin Jun 7, 2023

Choose a reason for hiding this comment

lgoettgens Jun 7, 2023

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

Add `hash` methods for many types with `==` method #2373

Add `hash` methods for many types with `==` method #2373

lgoettgens commented May 10, 2023 •

edited

Loading

codecov bot commented May 11, 2023 •

edited

Loading

lgoettgens May 17, 2023 •

edited

Loading

lgoettgens May 22, 2023 •

edited

Loading