-
Notifications
You must be signed in to change notification settings - Fork 89
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.
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
add isequal and hash methods to check tuple in BoundaryIndex Sets without type piracy #835
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Are there any performance differences?
For unordered Set:
PR
|
Co-authored-by: Knut Andreas Meyer <[email protected]>
Codecov ReportAttention:
❗ 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
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Perhaps add a changelog entry before merging
I just noticed the following, which I encountered in the p4est branch as well. If we hash now Edit: Nvm, the set operations seem fine as well as a small test with a dict |
Good point - did not think of this! I tried to understand what's going on, and AFAIU it works because That being said, I think we need @fredrikekre or @KristofferC to confirm that this way is ok. The rule for defining the hash: |
yeah I'm just unsure if the statement needs to hold the other way around too. So should |
At least the docstring for |
This does not imply the reverse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Co-authored-by: Knut Andreas Meyer <[email protected]>
See https://julialang.slack.com/archives/C01R8JU271D/p1698848270253179 slackhole inc