Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 22, 2024
Merged

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

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.

@github-actions GitHub Actions
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 •
edited
Loading

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.

tscrim
tscrim approved these changes Jan 18, 2024
View reviewed changes
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tscrim tscrim tscrim approved these changes

Assignees
No one assigned
Labels
c: number fields t: bug
Projects
None yet
Milestone
sage-10.3
Development

Successfully merging this pull request may close these issues.

wrong result for .is_integral() in quadratic field
4 participants
@pjbruin @tscrim @vbraun @mkoeppe