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

support calling PARI's qfbcornacchia() from BinaryQF #35262

Merged
merged 4 commits into from
Apr 1, 2023

Conversation

yyyyx4
Copy link
Member

@yyyyx4 yyyyx4 commented Mar 11, 2023

Recent versions of PARI have a qfbcornacchia() function which can replace special cases of qfbsolve() while being much faster. In this patch we add a path to qfbcornacchia() in the BinaryQF.solve_integer() method; it can be selected using an optional algorithm= argument. See the added doctest for an example of the speedup.

Also fixes #35292.

@codecov-commenter
Copy link

codecov-commenter commented Mar 11, 2023

Codecov Report

Patch coverage: 60.00% and project coverage change: -0.02 ⚠️

Comparison is base (c00e6c2) 88.62% compared to head (c9cb3c3) 88.61%.

❗ Current head c9cb3c3 differs from pull request most recent head e99fb4b. Consider uploading reports for the commit e99fb4b to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #35262      +/-   ##
===========================================
- Coverage    88.62%   88.61%   -0.02%     
===========================================
  Files         2148     2148              
  Lines       398855   398665     -190     
===========================================
- Hits        353480   353263     -217     
- Misses       45375    45402      +27     
Impacted Files Coverage Δ
src/sage/quadratic_forms/binary_qf.py 91.54% <60.00%> (-1.76%) ⬇️

... and 118 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@yyyyx4 yyyyx4 force-pushed the public/support_pari_qfbcornacchia branch from 6a8b260 to b7233c8 Compare March 13, 2023 04:28
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.

LGTM modulo some small things.

src/sage/quadratic_forms/binary_qf.py Outdated Show resolved Hide resolved
src/sage/quadratic_forms/binary_qf.py Outdated Show resolved Hide resolved
src/sage/quadratic_forms/binary_qf.py Show resolved Hide resolved
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.

SageMath version 10.0.beta5, Release Date: 2023-03-19
@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: e99fb4b

@vbraun vbraun merged commit 65c89ec into sagemath:develop Apr 1, 2023
@mkoeppe mkoeppe added this to the sage-10.0 milestone Apr 1, 2023
@yyyyx4 yyyyx4 deleted the public/support_pari_qfbcornacchia branch April 2, 2023 02:09
@GMS103
Copy link
Member

GMS103 commented Apr 8, 2023

See
#35292 (comment)
for a seemingly related problem.

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.

binary_qf tests fail for a particular random seed
6 participants