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

Fix NumberFieldElement_quadratic.is_integral() when D is not square-free #37082

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

pjbruin
Copy link
Contributor

@pjbruin pjbruin commented Jan 17, 2024

Fixes #34800. The current implementation of is_integral() assumes that D is square-free. We implement the check for integrality of the trace and norm in a way that works for general D.

Copy link

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

@tscrim
Copy link
Collaborator

tscrim commented Jan 18, 2024

Does it make sense to keep the old test for the square-free case (which, of course, would include a check in the method for that)?

@pjbruin
Copy link
Contributor Author

pjbruin commented Jan 18, 2024

Does it make sense to keep the old test for the square-free case (which, of course, would include a check in the method for that)?

The problem with that is that knowing whether D is square-free requires factoring D, which we should certainly avoid. In my implementation, we only need to check one specific candidate for a square divisor, namely denom^2 or (denom/2)^2 depending on a parity condition.

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the explanation. I wasn’t sure if this was already tested somewhere during the initialization (or could have been cached by some other computation) and if the new test would have been (relatively) slower than the old in the case it was already known.

Anyways, positive review.

@pjbruin
Copy link
Contributor Author

pjbruin commented Jan 18, 2024

Thank you!

@vbraun vbraun merged commit a5f01d4 into sagemath:develop Jan 22, 2024
21 checks passed
@mkoeppe mkoeppe added this to the sage-10.3 milestone Mar 7, 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.

wrong result for .is_integral() in quadratic field
4 participants