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:

Status badges

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 tscrim

  • Branch set to public/algebra/fix_comparisons_mpz_cmp-29094
  • Status changed from new to needs_review

comment:2 Changed 2 years ago by git

  • Commit set to a06ce6250be021adc2c007072a04f4a800243c07

Branch pushed to git repo; I updated commit sha1. New commits:

​a06ce62Using rich_to_bool_sgn instead of rich_to_bool with mpz_cmp.

comment:3 Changed 2 years ago by git

  • Commit changed from a06ce6250be021adc2c007072a04f4a800243c07 to 501d5afab6ae20b45974c166d870dd6fc3b1fa6d

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​501d5afUsing rich_to_bool_sgn instead of rich_to_bool with mpz_cmp.

comment:4 Changed 2 years ago by chapoton

looks good. But why are there 2 changes ?

comment:5 Changed 2 years ago by tscrim

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 chapoton

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 chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

comment:8 Changed 2 years ago by tscrim

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 vbraun

  • Branch changed from public/algebra/fix_comparisons_mpz_cmp-29094 to 501d5afab6ae20b45974c166d870dd6fc3b1fa6d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.