Skip to content

Conversation

RuchitJagodara
Copy link
Contributor

This patch fixes #37217. I have also added a doctest for this issue and
removed an wrong example.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.

⌛ Dependencies

Not applicable.

Copy link

github-actions bot commented Feb 4, 2024

Documentation preview for this PR (built with commit 75092ca; changes) is ready! 🎉

@RuchitJagodara RuchitJagodara marked this pull request as draft February 4, 2024 11:48
@tscrim
Copy link
Collaborator

tscrim commented Feb 12, 2024

@pjbruin You might have some thoughts about this.

@pjbruin
Copy link
Contributor

pjbruin commented Feb 12, 2024

@RuchitJagodara At #37217 (comment) you wrote "it was failing for some cases"; can you clarify if this is true for the current state of this pull request, and for what cases?

@grhkm21
Copy link
Contributor

grhkm21 commented Feb 21, 2024

Is there any progress? Another PR is failing due to this bug

@RuchitJagodara
Copy link
Contributor Author

@RuchitJagodara At #37217 (comment) you wrote "it was failing for some cases"; can you clarify if this is true for the current state of this pull request, and for what cases?

Yes, this is true for the current state of the pull request, and I am not understanding where the exact problem lies. Initially, I misunderstood the algorithm, and as a result, the changes I made in this PR, which also seem incorrect.

Is there any progress? Another PR is failing due to this bug

Sorry because of my mid-sem exams I was not able to read these comments, 😞

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 11, 2024
sagemathgh-37644: Corrections in `.normalize_basis_at_p` and `.maximal_order()` of `quaternion_algebra.py`
    
- Corrected in `.normalize_basis_at_p` the wrong assignments of `f0,
f1`: The idea is to replace `f0` by `f0 + f1` and `f1` by `f0`
_simultaneously_ - the original code, however, first replaced `f0` by
`f0 + f1` and then copied the value to `f1`, hence causing a decrease in
dimension
- Corrected an index error just before the recursive call of
`normalize_basis_at_p` in the off-diagnonal case for `p = 2` (the
`else`-block)
- In `.maximal_order()` the intermediate basis `e_n` might not define an
order anymore (as seen in the comments, this was known to the author(s)
of the method) - therefore we need to manually compute the discriminant
in the loop instead of relying on the `.discriminant()`-method of
quaternion orders
- Adapted an example in `.maximal_order()` to use the new method
`.is_maximal()`

Fixes sagemath#37217 (both parts) and sagemath#37417.

Disclaimer:
I am aware of sagemath#37239, but since progress on that PR seems to have
halted, I took the liberty to fix the issues myself :)
    
URL: sagemath#37644
Reported by: Sebastian A. Spindler
Reviewer(s): Matthias Köppe
@S17A05
Copy link
Member

S17A05 commented Apr 12, 2024

I am closing this PR as the issue has been solved in 10.4beta3 via #37644.

@S17A05 S17A05 closed this Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quaternion algebra: bugs in maximal_order and normalize_basis_at_p
6 participants