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

Use rich_to_bool_sgn instead of rich_to_bool when comparing outputs of mpz_cmp #29094

Closed
tscrim opened this issue Jan 28, 2020 · 11 comments
Closed

Comments

@tscrim
Copy link
Collaborator

tscrim commented Jan 28, 2020

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

CC: @mmasdeu @fchapoton

Component: algebra

Keywords: comparison

Author: Travis Scrimshaw

Branch/Commit: 501d5af

Reviewer: Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/29094

@tscrim tscrim added this to the sage-9.1 milestone Jan 28, 2020
@tscrim
Copy link
Collaborator Author

tscrim commented Jan 28, 2020

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 28, 2020

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

a06ce62Using rich_to_bool_sgn instead of rich_to_bool with mpz_cmp.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 28, 2020

Commit: a06ce62

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 28, 2020

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 28, 2020

Changed commit from a06ce62 to 501d5af

@fchapoton
Copy link
Contributor

comment:4

looks good. But why are there 2 changes ?

@tscrim
Copy link
Collaborator Author

tscrim commented Jan 28, 2020

comment:5

That second change is because the documentation says it returns [-1, 0, 1], but test might not be in that range.

@fchapoton
Copy link
Contributor

comment:6

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 ?

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@tscrim
Copy link
Collaborator Author

tscrim commented Jan 28, 2020

comment:8

Doesn't seem like it, but I will have to investigate further.

Thank you for doing the review.

@vbraun
Copy link
Member

vbraun commented Jan 31, 2020

Changed branch from public/algebra/fix_comparisons_mpz_cmp-29094 to 501d5af

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants