Opened 2 years ago
Closed 2 years ago
#29094 closed defect (fixed)
Use rich_to_bool_sgn instead of rich_to_bool when comparing outputs of mpz_cmp
Reported by: | tscrim | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-9.1 |
Component: | algebra | Keywords: | comparison |
Cc: | mmasdeu, chapoton | Merged in: | |
Authors: | Travis Scrimshaw | Reviewers: | Frédéric Chapoton |
Report Upstream: | N/A | Work issues: | |
Branch: | 501d5af (Commits, GitHub, GitLab) | Commit: | 501d5afab6ae20b45974c166d870dd6fc3b1fa6d |
Dependencies: | Stopgaps: |
Description
According to its documentation, mpz_cmp
yields arbitrary positive/negative numbers, but rich_to_bool
assumes the input is in [-1, 0, 1]
. Thus it yields wrong results:
sage: B.<i,j,k> = QuaternionAlgebra(6) sage: B(1) == B(-1) False sage: B(1) != B(-1) False
Change History (9)
comment:1 Changed 2 years ago by
- Branch set to public/algebra/fix_comparisons_mpz_cmp-29094
- Status changed from new to needs_review
comment:2 Changed 2 years ago by
- Commit set to a06ce6250be021adc2c007072a04f4a800243c07
comment:3 Changed 2 years ago by
- Commit changed from a06ce6250be021adc2c007072a04f4a800243c07 to 501d5afab6ae20b45974c166d870dd6fc3b1fa6d
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
501d5af | Using rich_to_bool_sgn instead of rich_to_bool with mpz_cmp.
|
comment:4 Changed 2 years ago by
looks good. But why are there 2 changes ?
comment:5 Changed 2 years ago by
That second change is because the documentation says it returns [-1, 0, 1]
, but test
might not be in that range.
comment:6 Changed 2 years ago by
ok, you can set to positive as soon as the lights are green
by the way, the related #28945 seems not to work with the latest beta. Maybe the same kind of issue ?
comment:7 Changed 2 years ago by
- Reviewers set to Frédéric Chapoton
- Status changed from needs_review to positive_review
comment:8 Changed 2 years ago by
Doesn't seem like it, but I will have to investigate further.
Thank you for doing the review.
comment:9 Changed 2 years ago by
- Branch changed from public/algebra/fix_comparisons_mpz_cmp-29094 to 501d5afab6ae20b45974c166d870dd6fc3b1fa6d
- Resolution set to fixed
- Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Using rich_to_bool_sgn instead of rich_to_bool with mpz_cmp.