-
Notifications
You must be signed in to change notification settings - Fork 162
Add hash functions for group elements #5479
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
base: master
Are you sure you want to change the base?
Conversation
src/Groups/types.jl
Outdated
| const SubPcGroupElem = BasicGAPGroupElem{SubPcGroup} | ||
|
|
||
| function Base.hash(x::Union{PcGroupElem,SubPcGroupElem}, h::UInt) | ||
| return hash(letters(x), hash(parent(x), h)) |
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.
Just been talking about this with @jamesnohilly -- he pointed out that hash(parent(x), h) currently just returns h but that he included this because it seems sensible, to which I generally agree. However, I've then been thinking: I believe we allow comparing a PcGroupElem to a SubPcGroupElem if both come from the same "full" group -- is that right, @ThomasBreuer ? If so, then one may have g == h being true, where one is a PcGroupElem and then other a SubPcGroupElem -- but they have different parent groups. This then of course would lead to a correctness issue if those two parents eventually do get a working hash method, assuming that hash method would produce different results...
So perhaps we should not take the parent into account, but rather full_group(parent(x)).
But perhaps the real issue is that we allow comparing a PcGroupElem with a SubPcGroupElem in the first place... for rings, we don't generally allow comparing elements with different parents (we have a bunch of open PRs for AA/Nemo to make that stricter, another stalled discussion... sigh)
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.
Concerning comparisons of group elements with different parents:
We support this whenever the parents are "compatible", which roughly means that there are natural embeddings of the two parents into a common group.
For example, permutations of the same degree have compatible parents,
and PcGroupElems and SubPcGroupElems with same full group have compatible parents.
If we want to use parent information for the hash value, the full group of a PcGroupElem or SubPcGroupElem would logically be the right object.
Analogously, the degree (which is not known on the GAP side) could then be used for the hash of PermGroupElems.
src/Groups/types.jl
Outdated
| b = UInt(GAPWrap.HashPermutation(GapObj(x))) | ||
| return xor(h, b) |
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.
The second argument to GAP's HashKeyBag is actually also a seed. So we could have something like this:
| b = UInt(GAPWrap.HashPermutation(GapObj(x))) | |
| return xor(h, b) | |
| return UInt(GAPWrap.HashPermutation(GapObj(x), GapInt(h))) |
and then adjust HashPermutation suitably.
And of course a coupld tests in test/Group/ would be nice -- just compute the hash of a couple permutations etc., and e.g. create (1,2) twice and make sure both copies have the same hash, and different from that of (). Or so
This is not true. We changed hashing for GapObj to throw an error, cf https://github.com/oscar-system/GAP.jl/blob/a4faccc28634dc3c6156e90ead9cbaef4aafc70d/src/adapter.jl#L417 But since I still agree that the hashing in Oscar should make use of the composition of different |
For this specifically, I think the context was in relation to (sub)types of However I agree this could be something that might cause further issues. |
|
Concerning
|
|
ah, yeah, I was confused by the similarity of the terms I think this is in a position now where it could be un-drafted to get a final round of reviews. |
src/Groups/types.jl
Outdated
|
|
||
| function Base.hash(x::Union{PcGroupElem,SubPcGroupElem}, h::UInt) | ||
| G = full_group(parent(x))[1] | ||
| return hash(letters(x), hash(G, h)) |
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.
If we think of groups with large relative orders then letters(x) can be expected to be a long list.
syllables(x) would usually be shorter.
Both letters and syllables create new objects. Ideally we would take the internally stored exponent vector; for that, a GAP function analogous to HashPermutation would be needed.
ThomasBreuer
left a comment
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.
Tests for hashing PcGroupElem, SubPcGroupElem, MatrixGroupElem are missing.
This PR adds hash functions for group elements (see #5340).