Skip to content

Hash fraction_field_elements more appropriately #40019

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

Merged
merged 9 commits into from
Jun 1, 2025

Conversation

nbruin
Copy link
Contributor

@nbruin nbruin commented Apr 28, 2025

Equip various domains with a canoncial_associate, which finds unique representatives in some rings modulo the action of the unit group. For fields this is easy: there are only two orbits: 0 and the units (where 1 is a representative) and in ZZ we can just use "abs". For polynomial rings we can extend representatives by normalizing the leading coefficient to a canonical associate.

The normalization is important to find unique representatives of fraction field elements (because multplying both numerator and denominator by a unit doesn't change the represented class), which is necessary for hashing them. The library uses hashes of fraction field elements quite a bit, but luckily only in cases where we can actually normalize denominators (in fact, "reduce" always seems to fail for such rings, so the hash would already error out)

Fixes #35238

📝 Checklist

  • The title is concise and informative.
  • 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 and checked the documentation preview.

Copy link

github-actions bot commented Apr 28, 2025

Documentation preview for this PR (built with commit 7e499fc; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@nbruin
Copy link
Contributor Author

nbruin commented Apr 29, 2025

A slightly different approach would be to enhance reduce to normalize the denominator (and consequently have reduce fail if canonical_associate isn't available. The latter doesn't seem to get triggered (because in those cases, a good gcd wouldn't have been available and reduce would already fail), but it does change the output of a lot of doctests. Fraction fields get used a lot!!!

@mantepse
Copy link
Contributor

One thing to consider (I guess you did, but let me make it explicit): reduce stores its result (by modifying self) and remembers that self is reduced. Perhaps this should also be done for the normalization.

@nbruin
Copy link
Contributor Author

nbruin commented Apr 29, 2025

One thing to consider (I guess you did, but let me make it explicit): reduce stores its result (by modifying self) and remembers that self is reduced. Perhaps this should also be done for the normalization.

From what I've seen, I strongly suspect we can only reduce in cases where we can also normalize. "Reduce" as it is performed now isn't particularly well-defined and I suspect it was always meant to be "normalize". So if we go that route we should just normalize the denominator in reduce. That basically worked (I tested on some doctests) but it changes the printed output of a lot of doctests. I think it's a worthwhile step to take but the number of doctests to be adjusted makes it a big undertaking.

Another optimization would be to cache the hash, but I don't know how often a single element gets hashed in its lifetime.

@nbruin nbruin requested a review from saraedum April 29, 2025 20:12
@nbruin
Copy link
Contributor Author

nbruin commented May 8, 2025

@saraedum PING I thought getting this one fixed was time critical? I think this is ready to go in. Since you initiated a (different) fix, I think you'd be qualified to judge if the fix here is OK or if it is likely to have some unintended side effects, so your opinion would be highly appreciated.

Copy link
Contributor

@mantepse mantepse left a comment

Choose a reason for hiding this comment

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

LGTM. If @saraedum does not oppose, I think this should go in.

@nbruin
Copy link
Contributor Author

nbruin commented May 16, 2025

OK, seeing a positive review and no opposition, I'll move this to "positive review".

@nbruin
Copy link
Contributor Author

nbruin commented May 17, 2025

A labelling war with the github-actions bot?

vbraun pushed a commit to vbraun/sage that referenced this pull request May 18, 2025
sagemathgh-40019: Hash fraction_field_elements more appropriately
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Equip various domains with a `canoncial_associate`, which finds unique
representatives in some rings modulo the action of the unit group. For
fields this is easy: there are only two orbits: 0 and the units (where 1
is a representative) and in ZZ we can just use "abs". For polynomial
rings we can extend representatives by normalizing the leading
coefficient to a canonical associate.

The normalization is important to find unique representatives of
fraction field elements (because multplying both numerator and
denominator by a unit doesn't change the represented class), which is
necessary for hashing them. The library uses hashes of fraction field
elements quite a bit, but luckily only in cases where we can actually
normalize denominators (in fact, "reduce" always seems to fail for such
rings, so the hash would already error out)

Fixes sagemath#35238

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [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.
- [x] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#40019
Reported by: nbruin
Reviewer(s): Martin Rubey
vbraun pushed a commit to vbraun/sage that referenced this pull request May 24, 2025
sagemathgh-40019: Hash fraction_field_elements more appropriately
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Equip various domains with a `canoncial_associate`, which finds unique
representatives in some rings modulo the action of the unit group. For
fields this is easy: there are only two orbits: 0 and the units (where 1
is a representative) and in ZZ we can just use "abs". For polynomial
rings we can extend representatives by normalizing the leading
coefficient to a canonical associate.

The normalization is important to find unique representatives of
fraction field elements (because multplying both numerator and
denominator by a unit doesn't change the represented class), which is
necessary for hashing them. The library uses hashes of fraction field
elements quite a bit, but luckily only in cases where we can actually
normalize denominators (in fact, "reduce" always seems to fail for such
rings, so the hash would already error out)

Fixes sagemath#35238

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [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.
- [x] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#40019
Reported by: nbruin
Reviewer(s): Martin Rubey
vbraun pushed a commit to vbraun/sage that referenced this pull request May 26, 2025
sagemathgh-40019: Hash fraction_field_elements more appropriately
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Equip various domains with a `canoncial_associate`, which finds unique
representatives in some rings modulo the action of the unit group. For
fields this is easy: there are only two orbits: 0 and the units (where 1
is a representative) and in ZZ we can just use "abs". For polynomial
rings we can extend representatives by normalizing the leading
coefficient to a canonical associate.

The normalization is important to find unique representatives of
fraction field elements (because multplying both numerator and
denominator by a unit doesn't change the represented class), which is
necessary for hashing them. The library uses hashes of fraction field
elements quite a bit, but luckily only in cases where we can actually
normalize denominators (in fact, "reduce" always seems to fail for such
rings, so the hash would already error out)

Fixes sagemath#35238

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [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.
- [x] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#40019
Reported by: nbruin
Reviewer(s): Martin Rubey
vbraun pushed a commit to vbraun/sage that referenced this pull request May 28, 2025
sagemathgh-40019: Hash fraction_field_elements more appropriately
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Equip various domains with a `canoncial_associate`, which finds unique
representatives in some rings modulo the action of the unit group. For
fields this is easy: there are only two orbits: 0 and the units (where 1
is a representative) and in ZZ we can just use "abs". For polynomial
rings we can extend representatives by normalizing the leading
coefficient to a canonical associate.

The normalization is important to find unique representatives of
fraction field elements (because multplying both numerator and
denominator by a unit doesn't change the represented class), which is
necessary for hashing them. The library uses hashes of fraction field
elements quite a bit, but luckily only in cases where we can actually
normalize denominators (in fact, "reduce" always seems to fail for such
rings, so the hash would already error out)

Fixes sagemath#35238

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [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.
- [x] I have updated the documentation and checked the documentation
preview.
    
URL: sagemath#40019
Reported by: nbruin
Reviewer(s): Martin Rubey
@vbraun vbraun merged commit e13ce52 into sagemath:develop Jun 1, 2025
11 of 18 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.

Hash for fraction field element is too permissive
3 participants