-
Notifications
You must be signed in to change notification settings - Fork 41
Always re-solve eq in ProximalProjection before first computation of value and jacobian #1842
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
Conversation
…value and jacobian
Memory benchmark result| Test Name | %Δ | Master (MB) | PR (MB) | Δ (MB) | Time PR (s) | Time Master (s) |
| -------------------------------------- | ------------ | ------------------ | ------------------ | ------------ | ------------------ | ------------------ |
test_objective_jac_w7x | 6.25 % | 3.843e+03 | 4.084e+03 | 240.34 | 33.94 | 31.23 |
- test_proximal_jac_w7x_with_eq_update | 19.83 % | 5.713e+03 | 6.846e+03 | 1133.09 | 160.30 | 53.58 |
test_proximal_freeb_jac | -0.23 % | 1.320e+04 | 1.317e+04 | -30.03 | 77.52 | 77.72 |
test_proximal_freeb_jac_blocked | -0.66 % | 7.666e+03 | 7.615e+03 | -50.71 | 68.59 | 68.60 |
test_proximal_freeb_jac_batched | 0.62 % | 7.595e+03 | 7.642e+03 | 47.35 | 68.07 | 68.99 |
test_proximal_jac_ripple | 2.79 % | 7.475e+03 | 7.683e+03 | 208.23 | 69.41 | 71.33 |
test_proximal_jac_ripple_spline | 0.09 % | 3.471e+03 | 3.474e+03 | 3.23 | 71.30 | 71.22 |
test_eq_solve | -3.31 % | 2.073e+03 | 2.004e+03 | -68.59 | 125.16 | 125.07 |For the memory plots, go to the summary of |
| benchmark_name | dt(%) | dt(s) | t_new(s) | t_old(s) |
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
test_build_transform_fft_midres | +0.22 +/- 2.46 | +1.47e-03 +/- 1.63e-02 | 6.65e-01 +/- 8.3e-03 | 6.63e-01 +/- 1.4e-02 |
test_build_transform_fft_highres | -0.15 +/- 3.87 | -1.36e-03 +/- 3.50e-02 | 9.02e-01 +/- 2.6e-02 | 9.03e-01 +/- 2.4e-02 |
test_equilibrium_init_lowres | -2.88 +/- 1.79 | -1.29e-01 +/- 8.00e-02 | 4.34e+00 +/- 4.9e-02 | 4.46e+00 +/- 6.3e-02 |
test_objective_compile_atf | -3.50 +/- 3.62 | -2.18e-01 +/- 2.25e-01 | 6.00e+00 +/- 1.6e-01 | 6.22e+00 +/- 1.6e-01 |
test_objective_compute_atf | -7.20 +/- 17.63 | -1.59e-04 +/- 3.89e-04 | 2.05e-03 +/- 2.5e-04 | 2.21e-03 +/- 3.0e-04 |
test_objective_jac_atf | -1.62 +/- 2.26 | -2.80e-02 +/- 3.90e-02 | 1.70e+00 +/- 2.4e-02 | 1.73e+00 +/- 3.0e-02 |
test_perturb_1 | -4.57 +/- 3.00 | -6.48e-01 +/- 4.26e-01 | 1.35e+01 +/- 3.7e-01 | 1.42e+01 +/- 2.1e-01 |
test_proximal_jac_atf | -0.52 +/- 1.47 | -2.89e-02 +/- 8.23e-02 | 5.55e+00 +/- 6.6e-02 | 5.58e+00 +/- 4.9e-02 |
test_proximal_freeb_compute | -1.24 +/- 2.95 | -2.02e-03 +/- 4.79e-03 | 1.60e-01 +/- 3.2e-03 | 1.62e-01 +/- 3.6e-03 |
test_solve_fixed_iter | -3.89 +/- 2.33 | -1.15e+00 +/- 6.89e-01 | 2.84e+01 +/- 2.9e-01 | 2.95e+01 +/- 6.2e-01 |
test_objective_compute_ripple | -0.34 +/- 1.04 | -8.93e-03 +/- 2.72e-02 | 2.61e+00 +/- 2.0e-02 | 2.61e+00 +/- 1.9e-02 |
test_objective_grad_ripple | -0.37 +/- 2.48 | -1.76e-02 +/- 1.17e-01 | 4.68e+00 +/- 4.6e-02 | 4.69e+00 +/- 1.1e-01 | |
…/DESC into dp/prox-always-solve-first
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1842 +/- ##
==========================================
- Coverage 95.81% 95.81% -0.01%
==========================================
Files 100 100
Lines 27536 27539 +3
==========================================
+ Hits 26384 26386 +2
- Misses 1152 1153 +1
🚀 New features to boost your workflow:
|
…art and some are lower res so eq may be bad anyways)
YigitElma
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't see how we can prevent the failing benchmarks since the kwarg is recently added. If you can merge it by overriding somehow, I am fine with it.
Should update the changelog though
Reruns the optimization to see changes due to #1842.
Resolves #1831