add isequal and hash methods to check tuple in BoundaryIndex Sets without type piracy

[koehlerson]

See https://julialang.slack.com/archives/C01R8JU271D/p1698848270253179 slackhole inc

[KnutAM]

Nice!

Are there any performance differences?

[koehlerson]

Nice!

Are there any performance differences?

For unordered Set:
master

julia> @benchmark in((1,2),testset)
BenchmarkTools.Trial: 10000 samples with 989 evaluations.
 Range (min … max):  48.218 ns … 69.210 ns  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     48.507 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   48.717 ns ±  0.988 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

  ▆▆█▇█▆▄                              ▁ ▁▁                   ▂
  ███████▆▃▃▁▄▄▅▃▅▄▅▄▃▁▅▃▃▄▃▁▄▄▅▄▄▁▅▆▇█████▇▇█▇██▆▇▇▇▆▇▇▆▆▆▆▆ █
  48.2 ns      Histogram: log(frequency) by time        53 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

PR

julia> @benchmark in((1,2),testset)
BenchmarkTools.Trial: 10000 samples with 994 evaluations.
 Range (min … max):  31.995 ns … 63.816 ns  ┊ GC (min … max): 0.00% … 0.00%
 Time  (median):     32.391 ns              ┊ GC (median):    0.00%
 Time  (mean ± σ):   32.565 ns ±  1.520 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

  ▃  █▃                                                       ▁
  █▅▃██▆▃▃▁▁▁▃▁▁▁▁▁▁▃▁▁▁▁▁▁▁▃▁▁▁▁▁▃▁▁▁▁▁▃▄▃▃▆▅▄▆▅▄▄▅▅▅▅▃▄▇▄▃▄ █
  32 ns        Histogram: log(frequency) by time      38.6 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.
 

[codecov-commenter]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (fe44e7f) 92.67% compared to head (7e9837d) 92.98%.
Report is 4 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #835      +/-   ##
==========================================
+ Coverage   92.67%   92.98%   +0.31%     
==========================================
  Files          33       33              
  Lines        4955     4950       -5     
==========================================
+ Hits         4592     4603      +11     
+ Misses        363      347      -16     

Files	Coverage Δ
src/Grid/grid.jl	92.80% <75.00%> (-0.32%)	⬇️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[KnutAM]

LGTM!

Perhaps add a changelog entry before merging

[koehlerson]

I just noticed the following, which I encountered in the p4est branch as well. If we hash now FaceIndex(1,2) and EdgeIndex(1,2) the hash will be the same. This can be problematic if you have Set{Union{FaceIndex,EdgeIndex}} and do some set operations (which I think rely on the hashes and not only the isequal operation). In the p4est branch I just wrapped the Tuple{Int,Int} in a Tuple{Int,Tuple{Int,Int}} where the first int is sort of a category integer, but not sure if relevant

Edit: Nvm, the set operations seem fine as well as a small test with a dict

[KnutAM]

Good point - did not think of this!

I tried to understand what's going on, and AFAIU it works because isequal(FaceIndex(1,2), EdgeIndex(1,2)) is false, such that the following check give false and the loop continues until eventually returning a negative index (indicating that the key wasn't found), for a set without FaceIndex(1,2) but with EdgeIndex(1,2).

https://github.com/JuliaLang/julia/blob/6d4f40957c6e68ecfd1dc13115beadff493406fd/base/dict.jl#L289

That being said, I think we need @fredrikekre or @KristofferC to confirm that this way is ok. The rule for defining the hash: "Compute an integer hash code such that isequal(x,y) implies hash(x)==hash(y)", use implies, so to my understanding the implementation in this PR follows the docstring.

[koehlerson]

yeah I'm just unsure if the statement needs to hold the other way around too. So should hash(x) == hash(y) imply isequal(x,y). If so, then we should change it.

[KnutAM]

yeah I'm just unsure if the statement needs to hold the other way around too. So should hash(x) == hash(y) imply isequal(x,y). If so, then we should change it.

At least the docstring for isequal is consistent with our interpretation: "isequal(x,y) must imply that hash(x) == hash(y)."

[termi-official]

yeah I'm just unsure if the statement needs to hold the other way around too. So should hash(x) == hash(y) imply isequal(x,y). If so, then we should change it.

At least the docstring for isequal is consistent with our interpretation: "isequal(x,y) must imply that hash(x) == hash(y)."

This does not imply the reverse.

[fredrikekre]

This is fine. You would still see strange things for e.g. Set{Union{FaceIndex, Tuple{Int,Int}}} where there is no way to differentiate FaceIndex((1, 2)) and (1, 2). I don't think such mixed sets are so common though.