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

quaternion ideals' .scale() incorrectly copies cached left and right orders #32726

Closed
yyyyx4 opened this issue Oct 20, 2021 · 13 comments
Closed

Comments

@yyyyx4
Copy link
Member

yyyyx4 commented Oct 20, 2021

The method QuaternionFractionalIdeal_rational.scale() always copies over the existing left and right orders of self to the scaled ideal. This is incorrect when the scaling factor does not lie in the center of the algebra, as demonstrated by this example:

sage: Quat.<i,j,k> = QuaternionAlgebra(-1,-19)
sage: a = 1+2*i+3*j+4*k
sage: I = Quat.maximal_order().unit_ideal()
sage: I.right_order()
Order of Quaternion Algebra (-1, -19) with base ring Rational Field with basis (1/2 + 1/2*j, 1/2*i + 1/2*k, j, k)
sage: I.scale(a).right_order()
Order of Quaternion Algebra (-1, -19) with base ring Rational Field with basis (1/2 + 1/2*j, 1/2*i + 1/2*k, j, k)
sage: J = Quat.ideal(I.scale(a).basis(), check=False)
sage: J == I.scale(a)
True
sage: J.right_order()
Order of Quaternion Algebra (-1, -19) with base ring Rational Field with basis (1/2 + 1/10*j + 109/5*k, 1/48*i + 1/120*j + 8411/240*k, 1/5*j + 218/5*k, 120*k)

The patch fixes this by only copying over the cached orders when scaling on the other side, or when scaling by an element of QQ.

CC: @pjbruin

Component: algebra

Stopgaps: mathematically_wrong

Author: Lorenz Panny

Branch/Commit: 4310b6c

Reviewer: Michael Orlitzky

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

@yyyyx4 yyyyx4 added this to the sage-9.5 milestone Oct 20, 2021
@yyyyx4
Copy link
Member Author

yyyyx4 commented Oct 20, 2021

Branch: public/32726

@yyyyx4

This comment has been minimized.

@yyyyx4
Copy link
Member Author

yyyyx4 commented Oct 20, 2021

Author: Lorenz Panny

@yyyyx4
Copy link
Member Author

yyyyx4 commented Oct 20, 2021

Commit: ef98f79

@yyyyx4
Copy link
Member Author

yyyyx4 commented Oct 21, 2021

comment:2

Bumping priority since this bug can lead to mathematical errors.

@mkoeppe
Copy link
Contributor

mkoeppe commented Dec 18, 2021

comment:3

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.

@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 18, 2022

Changed commit from ef98f79 to 4310b6c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 18, 2022

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

4310b6cMerge tag '9.5.rc2' into public/32726

@orlitzky
Copy link
Contributor

comment:5

This looks OK to me but I'm not an expert so I have two questions:

  1. I guess scaling on the right doesn't affect the left order, and vice-versa?
  2. Is it OK to assume that the base ring is QQ?

@yyyyx4
Copy link
Member Author

yyyyx4 commented Jan 24, 2022

comment:6

Thanks for having a look.

  1. Yes; the left order of I is {x in the algebra | x·I ⊆ I}. Scaling happens element-wise, so these inclusions are preserved under right scaling. Similarly on the other side.
  2. Yes; the class QuaternionFractionalIdeal_rational is specific to QQ.

@orlitzky
Copy link
Contributor

comment:7

LGTM then, thanks.

@orlitzky
Copy link
Contributor

Reviewer: Michael Orlitzky

@vbraun
Copy link
Member

vbraun commented Jan 31, 2022

Changed branch from public/32726 to 4310b6c

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