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

_richcmp_ for quaternion algebra elements #28398

Closed
fchapoton opened this issue Aug 25, 2019 · 10 comments
Closed

_richcmp_ for quaternion algebra elements #28398

fchapoton opened this issue Aug 25, 2019 · 10 comments

Comments

@fchapoton
Copy link
Contributor

instead of the old-style _cmp_

CC: @tscrim @fchapoton

Component: algebra

Author: Frédéric Chapoton

Branch: c8425cb

Reviewer: Travis Scrimshaw

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

@fchapoton fchapoton added this to the sage-8.9 milestone Aug 25, 2019
@fchapoton
Copy link
Contributor Author

Commit: c8425cb

@fchapoton
Copy link
Contributor Author

New commits:

c8425cbrichcmp for quaternion algebras elements

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/28398

@fchapoton
Copy link
Contributor Author

comment:2

bot is morally green, please review

@tscrim
Copy link
Collaborator

tscrim commented Aug 25, 2019

comment:3

LGTM.

@tscrim
Copy link
Collaborator

tscrim commented Aug 25, 2019

Reviewer: Travis Scrimshaw

@vbraun
Copy link
Member

vbraun commented Aug 28, 2019

Changed branch from u/chapoton/28398 to c8425cb

@mmasdeu
Copy link
Sponsor

mmasdeu commented Jan 28, 2020

comment:5

This breaks quaternion algebra comparisons quite badly. In Sage 9.0:

sage: B.<i,j,k> = QuaternionAlgebra(6)
sage: B(1) == B(-1)
False
sage: B(1) != B(-1)
False

@mmasdeu
Copy link
Sponsor

mmasdeu commented Jan 28, 2020

Changed commit from c8425cb to none

@mmasdeu mmasdeu assigned tscrim and fchapoton and unassigned tscrim Jan 28, 2020
@tscrim
Copy link
Collaborator

tscrim commented Jan 28, 2020

comment:8

So this change did not cause that problem, but instead #28595. The problem comes from the fact that mpz_cmp yields arbitrary positive/negative numbers, but rich_to_bool assumes the input is in [-1, 0, 1]. Instead, we just need to use rich_to_bool_sgn. This is now #29094.

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

4 participants