Skip to content

BUGFIX: Update preconditioner in cg #1177

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

IvanBioli
Copy link

After the formatting of the repository in #1150 , a small error was introduced in the update_state! function in the src/multivariate/solvers/first_order/cg.jl file: the preconditioner is not updated at every iteration, but only at the first one. The change ensures that the _apply_precondprep method is called. Changes:

Copy link
Contributor

Benchmark Results

master 3f65f67... master / 3f65f67...
multivariate/solvers/first_order/AdaMax 0.643 ± 0.0082 ms 0.645 ± 0.008 ms 0.997 ± 0.018
multivariate/solvers/first_order/Adam 0.644 ± 0.008 ms 0.645 ± 0.0079 ms 0.998 ± 0.017
multivariate/solvers/first_order/BFGS 0.224 ± 0.0038 ms 0.224 ± 0.0043 ms 1 ± 0.026
multivariate/solvers/first_order/ConjugateGradient 0.0485 ± 0.00065 ms 0.0483 ± 0.00064 ms 1 ± 0.019
multivariate/solvers/first_order/GradientDescent 1.71 ± 0.014 ms 1.71 ± 0.012 ms 1 ± 0.011
multivariate/solvers/first_order/LBFGS 0.22 ± 0.0038 ms 0.219 ± 0.0083 ms 1 ± 0.042
multivariate/solvers/first_order/MomentumGradientDescent 2.51 ± 0.018 ms 2.5 ± 0.012 ms 1 ± 0.0086
multivariate/solvers/first_order/NGMRES 0.555 ± 0.011 ms 0.555 ± 0.011 ms 1 ± 0.028
time_to_load 0.525 ± 0.0036 s 0.529 ± 0.0022 s 0.993 ± 0.008

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@pkofod
Copy link
Member

pkofod commented Jul 17, 2025

Thanks for noticing, can you link to the line in question? I don't see any changes to cg.jl in that pr :)

Copy link

codecov bot commented Jul 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.71%. Comparing base (6850998) to head (3f65f67).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1177   +/-   ##
=======================================
  Coverage   85.70%   85.71%           
=======================================
  Files          46       46           
  Lines        3596     3597    +1     
=======================================
+ Hits         3082     3083    +1     
  Misses        514      514           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@IvanBioli
Copy link
Author

Thanks for noticing, can you link to the line in question? I don't see any changes to cg.jl in that pr :)

I added _apply_precondprep(method, state.x) at line 187

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants