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.

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

Added hash function to elements of the fundamental group of an extended affine Weyl group #36122

Merged
merged 5 commits into from
Sep 16, 2023

Conversation

65537
Copy link
Contributor

@65537 65537 commented Aug 22, 2023

This change allows users to compute the python hash function for elements of the (Borovoi) fundamental group of an extended affine Weyl group, and subsequently for elements of the extended Weyl group itself if using the FW realization. This, in turn, allows to use these mathematical objects as keys in dictionaries.

Fixes #36121 . It adds a __hash__ function to the class FundamentalGroupElement associated with FundamentalGroupOfExtendedAffineWeylGroup.

SAGE already associates to each element of the fundamental group an integer, accessible as its value() or _value. This integer is unique among other elements of the same fundamental group and thus suitable as a hash for hashmap applications (e.g. Python dictionaries). We return the hash of that integer, which is the integer itself for current python implementations.

W = ExtendedAffineWeylGroup('A4')
fun = W.fundamental_group().an_element()
w = W.FW().an_element()

hash(fun), {w: 0}

yields the following result:

(4, {pi[4] * S0*S1*S2*S3*S4: 0})

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

Calculates the hash from its value, which is an integer uniquely determining the fundamental group element
Maybe Linter likes me now
@github-actions
Copy link

Documentation preview for this PR (built with commit 8e9c87a; changes) is ready! 🎉

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Thank you for noticing and fixing this.

For next time, please be sure to add a doctest showing the issue is fixed (also, all methods need to have at least one doctest).

src/sage/combinat/root_system/fundamental_group.py Outdated Show resolved Hide resolved
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
@65537
Copy link
Contributor Author

65537 commented Sep 7, 2023

Dear Travis,

thank you for reviewing and improving my pull request. I was following the (mathematically very similar) class WeylGroupElement whose hash function has neither a docstring nor a test, but I will make sure to always include them in future pull requests.

Best, Felix

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

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

Indeed, that is generally a good idea. Unfortunately in this case that is some fairly old code that was included before we developed our modern practices. Thank you for adding it. LGTM.

@vbraun
Copy link
Member

vbraun commented Sep 10, 2023

sage -t --long --warn-long 39.2 --random-seed=123 src/sage/combinat/root_system/fundamental_group.py
**********************************************************************
File "src/sage/combinat/root_system/fundamental_group.py", line 339, in sage.combinat.root_system.fundamental_group.FundamentalGroupElement.__hash__
Failed example:
    hash(w) == hash(w.value())
Exception raised:
    Traceback (most recent call last):
      File "/home/release/Sage/src/sage/doctest/forker.py", line 709, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/release/Sage/src/sage/doctest/forker.py", line 1144, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.combinat.root_system.fundamental_group.FundamentalGroupElement.__hash__[3]>", line 1, in <module>
        hash(w) == hash(w.value())
                        ^^^^^^^^^
    TypeError: 'tuple' object is not callable
**********************************************************************

@tscrim
Copy link
Collaborator

tscrim commented Sep 10, 2023

@65537 Sorry, I didn’t check the doctest carefully. Please change value() to value.

Test for __hash__ should succeed now
Copy link
Contributor Author

@65537 65537 left a comment

Choose a reason for hiding this comment

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

Testing for value() is correct, as long as you are testing on FundamentalGroupElements, which was the main culprit here

@tscrim
Copy link
Collaborator

tscrim commented Sep 10, 2023

Ah, indeed.

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 11, 2023
…group of an extended affine Weyl group

    
This change allows users to compute the python `hash` function for
elements of the (Borovoi) fundamental group of an extended affine Weyl
group, and subsequently for elements of the extended Weyl group itself
if using the `FW` realization. This, in turn, allows to use these
mathematical objects as keys in dictionaries.

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

Fixes sagemath#36121 . It adds a `__hash__` function to the class
`FundamentalGroupElement` associated with
`FundamentalGroupOfExtendedAffineWeylGroup`.

SAGE already associates to each element of the fundamental group an
integer, accessible as its `value()` or `_value`.  This integer is
unique among other elements of the same fundamental group and thus
suitable as a hash for hashmap applications (e.g. Python dictionaries).
We return the `hash` of that integer, which is the integer itself for
current python implementations.

```
W = ExtendedAffineWeylGroup('A4')
fun = W.fundamental_group().an_element()
w = W.FW().an_element()

hash(fun), {w: 0}
```
yields the following result:
```
(4, {pi[4] * S0*S1*S2*S3*S4: 0})
```

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36122
Reported by: 65537
Reviewer(s): 65537, Travis Scrimshaw
@tscrim
Copy link
Collaborator

tscrim commented Sep 13, 2023

@vbraun Has this been merged? If so, should we close this?

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 14, 2023
…group of an extended affine Weyl group

    
This change allows users to compute the python `hash` function for
elements of the (Borovoi) fundamental group of an extended affine Weyl
group, and subsequently for elements of the extended Weyl group itself
if using the `FW` realization. This, in turn, allows to use these
mathematical objects as keys in dictionaries.

<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->

Fixes sagemath#36121 . It adds a `__hash__` function to the class
`FundamentalGroupElement` associated with
`FundamentalGroupOfExtendedAffineWeylGroup`.

SAGE already associates to each element of the fundamental group an
integer, accessible as its `value()` or `_value`.  This integer is
unique among other elements of the same fundamental group and thus
suitable as a hash for hashmap applications (e.g. Python dictionaries).
We return the `hash` of that integer, which is the integer itself for
current python implementations.

```
W = ExtendedAffineWeylGroup('A4')
fun = W.fundamental_group().an_element()
w = W.FW().an_element()

hash(fun), {w: 0}
```
yields the following result:
```
(4, {pi[4] * S0*S1*S2*S3*S4: 0})
```

<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: sagemath#36122
Reported by: 65537
Reviewer(s): 65537, Travis Scrimshaw
@vbraun vbraun merged commit 4553735 into sagemath:develop Sep 16, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing __hash__ function for elements of FundamentalGroupOfExtendedAffineWeylGroup
3 participants