-
-
Notifications
You must be signed in to change notification settings - Fork 649
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
Conversation
Documentation preview for this PR (built with commit 7e499fc; changes) is ready! 🎉 |
A slightly different approach would be to enhance |
One thing to consider (I guess you did, but let me make it explicit): |
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 Another optimization would be to cache the hash, but I don't know how often a single element gets hashed in its lifetime. |
Co-authored-by: Martin Rubey <axiomize@yahoo.de>
Co-authored-by: Martin Rubey <axiomize@yahoo.de>
@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. |
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. If @saraedum does not oppose, I think this should go in.
OK, seeing a positive review and no opposition, I'll move this to "positive review". |
A labelling war with the github-actions bot? |
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
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
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
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
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