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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in QF.find_primitive_p_divisible_vector__next #36551

Merged
merged 3 commits into from
Nov 30, 2023

Conversation

tornaria
Copy link
Contributor

When the vector [0,...,0,1] is a zero mod p, it was missed from the iteration. Fix by check and return it if this is the case.

A doctest triggering the bug is included.

The second commit adds a return_matrix option for QF.find_p_neighbor_from_vec which is related and useful, similar to QF.is_globally_equivalent_to, etc.

馃摑 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

@tornaria
Copy link
Contributor Author

tornaria commented Nov 9, 2023

Ping @yyyyx4 : if you find the time for a quick review, this is a very small PR that fixes a few annoying bugs in ternary quadratic forms. Most of the changed lines are new (simple) doctests for the bugs. Thanks.

@yyyyx4
Copy link
Member

yyyyx4 commented Nov 10, 2023

Your changes look good to me.

As a side remark (which is orthogonal to this patch), the API with __next here is really strange by Python standards: I suppose it should just be a generator, using yield.

When the vector [0,...,0,1] is a zero mod p, it was missed from the
iteration. Fix by check and return it if this is the case.

A doctest triggering the bug is included.
Similar to QF.is_globally_equivalent_to, etc.
This breaks TernaryQF.find_p_neighbor_from_vec().
Copy link

Documentation preview for this PR (built with commit 522edd1; changes) is ready! 馃帀

@tornaria
Copy link
Contributor Author

@vbraun I'd appreciate if this could be merged for 10.2, as it fixes two real annoying bugs and it's quite unobtrusive. I'm finishing a paper+code and it'd be nice if it works on unpatched 10.2.

@tornaria
Copy link
Contributor Author

Quoting #36031 (comment):

Marking this as a blocker in the hope it can sneak into 10.2 with the other open blocker

@mkoeppe
Copy link
Member

mkoeppe commented Nov 27, 2023

Yes, that's the documented mechanism to request that something be included in the release.

@vbraun vbraun merged commit c80adfa into sagemath:develop Nov 30, 2023
16 of 18 checks passed
@tornaria tornaria deleted the qf branch December 1, 2023 11:27
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.

None yet

4 participants