Skip to content

Conversation

@leburgel
Copy link
Contributor

The starting time t₀ was only recorded once at the start of the LBFGS run, and was kept constant throughout the rest of the iterations such that the printed time was the elapsed time since the start of the algorithm, not the time of the single iteration.

@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2025

Codecov Report

❌ Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.35%. Comparing base (e476e82) to head (3b8ec9b).
⚠️ Report is 9 commits behind head on master.

Files with missing lines Patch % Lines
src/OptimKit.jl 70.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
+ Coverage   82.06%   82.35%   +0.28%     
==========================================
  Files           5        5              
  Lines         513      527      +14     
==========================================
+ Hits          421      434      +13     
- Misses         92       93       +1     

☔ 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.

@Jutho
Copy link
Owner

Jutho commented Aug 13, 2025

I think this was actually intentional. See also the last code block after the loop:

    if _hasconverged
        verbosity >= 2 &&
            @info @sprintf("GD: converged after %d iterations and time %.2f s: f = %.12f, ‖∇f‖ = %.4e",
                           numiter, t, f, normgrad)
    else
        verbosity >= 1 &&
            @warn @sprintf("GD: not converged to requested tol after %d iterations and time %.2f s: f = %.12f, ‖∇f‖ = %.4e",
                           numiter, t, f, normgrad)
    end

Hence, t is supposed to represent the total time elapsed.

I agree it would make sense to show some Δt per time step instead of total time, but that requires something along the lines of

told = t
t = time() - t₀
Δt = t - told

Can you change PR accordingly, and maybe then make this uniform across LBFGS, CG and GD ?

@Jutho
Copy link
Owner

Jutho commented Oct 20, 2025

If you have time for this, it would be great to finish this and bundle this with the optimtest improvement in a new release. If not, then I can try to do it myself later this week.

@leburgel
Copy link
Contributor Author

Totally forgot about this. I'll give it a go later today.

@leburgel
Copy link
Contributor Author

I took the liberty of also updating the formatting of the time and cost function, since the current output sort of relied on both of these to be order 1 to be readable.

Co-authored-by: Jutho <Jutho@users.noreply.github.com>
@leburgel leburgel requested a review from Jutho October 22, 2025 13:19
@Jutho Jutho merged commit abd80ca into Jutho:master Oct 22, 2025
12 checks passed
@Jutho
Copy link
Owner

Jutho commented Oct 22, 2025

Thanks

@leburgel leburgel deleted the patch-1 branch October 22, 2025 14:01
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.

3 participants