Skip to content

Conversation

@devmotion
Copy link
Contributor

No description provided.

@pkofod
Copy link
Member

pkofod commented Nov 2, 2025

the tests that fail are quite sensitive / unstable

@devmotion
Copy link
Contributor Author

The failing macOS test uses the native aarch64 binaries in this PR but currently on master runs with Rosetta emulation.

@pkofod
Copy link
Member

pkofod commented Nov 2, 2025

I can make a commit to adjust the tests accordingly

- "min"
- "lts"
- "1"
- "pre"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit questionable whether to always run tests on "pre" - when there's no new prerelease available (e.g. for 1.13), then tests will be identical to "1".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it in a4c4adc

@devmotion
Copy link
Contributor Author

I can make a commit to adjust the tests accordingly

That would be great, I'm not familiar with that part of Optim yet.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2025

Benchmark Results (Julia vlts)

Time benchmarks
master b409f93... master / b409f93...
multivariate/solvers/first_order/AdaMax 0.546 ± 0.0092 ms 0.544 ± 0.0094 ms 1.01 ± 0.024
multivariate/solvers/first_order/Adam 0.546 ± 0.0092 ms 0.544 ± 0.0091 ms 1 ± 0.024
multivariate/solvers/first_order/BFGS 0.261 ± 0.0085 ms 0.26 ± 0.0082 ms 1 ± 0.046
multivariate/solvers/first_order/ConjugateGradient 0.175 ± 0.003 ms 0.174 ± 0.0031 ms 1.01 ± 0.025
multivariate/solvers/first_order/GradientDescent 1.55 ± 0.013 ms 1.53 ± 0.013 ms 1.01 ± 0.012
multivariate/solvers/first_order/LBFGS 0.232 ± 0.0074 ms 0.231 ± 0.0071 ms 1.01 ± 0.044
multivariate/solvers/first_order/MomentumGradientDescent 2.17 ± 0.017 ms 2.15 ± 0.018 ms 1.01 ± 0.012
multivariate/solvers/first_order/NGMRES 0.433 ± 0.011 ms 0.427 ± 0.011 ms 1.01 ± 0.036
time_to_load 0.41 ± 0.0023 s 0.415 ± 0.0051 s 0.989 ± 0.013
Memory benchmarks
master b409f93... master / b409f93...
multivariate/solvers/first_order/AdaMax 0.34 k allocs: 7.16 kB 0.34 k allocs: 7.16 kB 1
multivariate/solvers/first_order/Adam 0.34 k allocs: 7.16 kB 0.34 k allocs: 7.16 kB 1
multivariate/solvers/first_order/BFGS 0.36 k allocs: 15.5 kB 0.336 k allocs: 15 kB 1.04
multivariate/solvers/first_order/ConjugateGradient 0.362 k allocs: 14.2 kB 0.332 k allocs: 13.5 kB 1.05
multivariate/solvers/first_order/GradientDescent 2.09 k allocs: 0.0759 MB 1.89 k allocs: 0.0713 MB 1.06
multivariate/solvers/first_order/LBFGS 0.341 k allocs: 14.7 kB 0.317 k allocs: 14.2 kB 1.04
multivariate/solvers/first_order/MomentumGradientDescent 2.44 k allocs: 0.0815 MB 2.24 k allocs: 0.077 MB 1.06
multivariate/solvers/first_order/NGMRES 1.56 k allocs: 0.117 MB 1.51 k allocs: 0.117 MB 0.997
time_to_load 0.153 k allocs: 14.5 kB 0.153 k allocs: 14.5 kB 1

A plot of the benchmark results has been uploaded as an artifact at .

@codecov
Copy link

codecov bot commented Nov 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.80%. Comparing base (c3ba21c) to head (b409f93).
⚠️ Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1196      +/-   ##
==========================================
+ Coverage   87.59%   87.80%   +0.20%     
==========================================
  Files          45       45              
  Lines        3515     3517       +2     
==========================================
+ Hits         3079     3088       +9     
+ Misses        436      429       -7     

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

@devmotion
Copy link
Contributor Author

@pkofod You could release the bug fix in JuliaNLSolvers/NLSolversBase.jl#163 (I bumped the version in that PR and the changes up until that commit should be non-breaking) to see if it fixes the test failures here.

@pkofod
Copy link
Member

pkofod commented Nov 18, 2025

@pkofod You could release the bug fix in JuliaNLSolvers/NLSolversBase.jl#163 (I bumped the version in that PR and the changes up until that commit should be non-breaking) to see if it fixes the test failures here.

the constraints bug? Those two failures are not using constraints though

@devmotion
Copy link
Contributor Author

Ah, OK, I just saw that test failures were related to IPNewton.

@pkofod
Copy link
Member

pkofod commented Nov 19, 2025

Ah, OK, I just saw that test failures were related to IPNewton.

the failing tests here were NGMRES (in my experience quite unstable) and now it's momentum (also quite unstable). I spent some time checking the implementation and I think it is just unstable, maybe especially with a line search. It typically is used for large systems and "machine learning" so often you just have a small step size / "learning rate" or even some kind of decay structure that forces the procedure to settle down.

@pkofod
Copy link
Member

pkofod commented Nov 19, 2025

the failing tests here were NGMRES (in my experience quite unstable) and now it's momentum (also quite unstable). I spent some time checking the implementation and I think it is just unstable, maybe especially with a line search. It typically is used for large systems and "machine learning" so often you just have a small step size / "learning rate" or even some kind of decay structure that forces the procedure to settle down.

If the target is a breaking release anyway, we could force users to choose a line search here.

@pkofod
Copy link
Member

pkofod commented Nov 21, 2025

given the length of the skip list it may be time to find a better way to test this method :)

@pkofod pkofod merged commit 67320a2 into JuliaNLSolvers:master Nov 21, 2025
13 checks passed
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