Use single LinearConstraintProjection in ProximalProjection
#1409
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Add kwarg eq.solve which takes in |
…ke perturb and solve accept LinearConstraintProjection as objective
| benchmark_name | dt(%) | dt(s) | t_new(s) | t_old(s) |
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
test_build_transform_fft_lowres | -0.24 +/- 1.22 | -1.25e-03 +/- 6.33e-03 | 5.19e-01 +/- 4.1e-03 | 5.21e-01 +/- 4.8e-03 |
test_equilibrium_init_medres | +0.14 +/- 0.80 | +5.80e-03 +/- 3.26e-02 | 4.09e+00 +/- 2.4e-02 | 4.08e+00 +/- 2.2e-02 |
test_equilibrium_init_highres | -0.37 +/- 0.93 | -1.98e-02 +/- 4.99e-02 | 5.34e+00 +/- 3.3e-02 | 5.36e+00 +/- 3.8e-02 |
test_objective_compile_dshape_current | +0.18 +/- 4.38 | +7.16e-03 +/- 1.77e-01 | 4.05e+00 +/- 1.2e-01 | 4.04e+00 +/- 1.3e-01 |
test_objective_compute_dshape_current | -0.15 +/- 1.10 | -7.68e-06 +/- 5.64e-05 | 5.11e-03 +/- 4.0e-05 | 5.12e-03 +/- 3.9e-05 |
test_objective_jac_dshape_current | +0.76 +/- 7.32 | +3.19e-04 +/- 3.07e-03 | 4.23e-02 +/- 2.4e-03 | 4.20e-02 +/- 1.9e-03 |
test_perturb_2 | +0.03 +/- 0.65 | +6.56e-03 +/- 1.26e-01 | 1.94e+01 +/- 1.1e-01 | 1.94e+01 +/- 5.8e-02 |
+test_proximal_jac_atf_with_eq_update | -6.04 +/- 0.90 | -2.65e+00 +/- 3.95e-01 | 4.12e+01 +/- 1.7e-01 | 4.39e+01 +/- 3.6e-01 |
test_proximal_freeb_jac | -0.73 +/- 1.54 | -5.38e-02 +/- 1.13e-01 | 7.29e+00 +/- 9.9e-02 | 7.35e+00 +/- 5.4e-02 |
test_solve_fixed_iter_compiled | +0.49 +/- 2.45 | +9.77e-02 +/- 4.88e-01 | 2.01e+01 +/- 2.2e-01 | 2.00e+01 +/- 4.4e-01 |
test_LinearConstraintProjection_build | +2.95 +/- 2.84 | +3.02e-01 +/- 2.90e-01 | 1.05e+01 +/- 2.8e-01 | 1.02e+01 +/- 6.5e-02 |
test_build_transform_fft_midres | +3.72 +/- 3.09 | +2.36e-02 +/- 1.96e-02 | 6.57e-01 +/- 9.1e-03 | 6.33e-01 +/- 1.7e-02 |
test_build_transform_fft_highres | +3.22 +/- 2.72 | +3.17e-02 +/- 2.68e-02 | 1.02e+00 +/- 1.5e-02 | 9.85e-01 +/- 2.2e-02 |
-test_equilibrium_init_lowres | +8.65 +/- 2.60 | +3.50e-01 +/- 1.05e-01 | 4.40e+00 +/- 4.9e-02 | 4.05e+00 +/- 9.3e-02 |
test_objective_compile_atf | +3.80 +/- 2.80 | +3.25e-01 +/- 2.39e-01 | 8.87e+00 +/- 5.9e-02 | 8.54e+00 +/- 2.3e-01 |
test_objective_compute_atf | +6.09 +/- 4.04 | +9.67e-04 +/- 6.41e-04 | 1.68e-02 +/- 3.0e-04 | 1.59e-02 +/- 5.7e-04 |
test_objective_jac_atf | +0.29 +/- 1.44 | +5.80e-03 +/- 2.86e-02 | 1.98e+00 +/- 1.7e-02 | 1.98e+00 +/- 2.3e-02 |
test_perturb_1 | -4.12 +/- 3.48 | -6.70e-01 +/- 5.66e-01 | 1.56e+01 +/- 5.6e-01 | 1.63e+01 +/- 7.8e-02 |
+test_proximal_jac_atf | -3.00 +/- 0.89 | -2.56e-01 +/- 7.57e-02 | 8.27e+00 +/- 5.2e-02 | 8.52e+00 +/- 5.5e-02 |
+test_proximal_freeb_compute | -48.76 +/- 1.22 | -9.92e-02 +/- 2.48e-03 | 1.04e-01 +/- 1.0e-03 | 2.03e-01 +/- 2.3e-03 |
+test_solve_fixed_iter | -5.42 +/- 1.79 | -1.88e+00 +/- 6.21e-01 | 3.27e+01 +/- 5.1e-01 | 3.46e+01 +/- 3.5e-01 | |
YigitElma
commented
Jan 31, 2025
YigitElma
commented
Jan 31, 2025
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1409 +/- ##
==========================================
- Coverage 95.69% 95.68% -0.01%
==========================================
Files 101 101
Lines 25633 25694 +61
==========================================
+ Hits 24529 24585 +56
- Misses 1104 1109 +5
|
YigitElma
requested review from
a team and
rahulgaur104
and removed request for
a team
YigitElma
requested review from
f0uriest,
ddudt,
dpanici,
sinaatalay and
unalmis
… the same, especially for the first state
f0uriest
requested changes
Feb 2, 2025
| if hasattr(con, "update_target"): | ||
| con.update_target(self._eq) | ||
| if (x != self._allx[-1]).all(): | ||
| self._eq_solve_objective.update_constraint_target(self._eq) |
There was a problem hiding this comment.
i think this needs to be inside the else block above. if we reset the equilibrium we should reset the constraints as well
There was a problem hiding this comment.
I am not sure I understand the previous if statement completely. If store=True then we store the intermediate equilibrium in history, but if it is False, history only has the first params_dict, right? Why do we go back to the initial one?
| if linear_constraint is not None and nonlinear_constraint is not None: | ||
| objective, linear_constraint, nonlinear_constraint = combine_args( | ||
| objective, linear_constraint, nonlinear_constraint | ||
| if not is_linear_proj: |
There was a problem hiding this comment.
might be worth moving all the stuff in this block to a helper function rather than adding another level of conditionals ie
if not is_linear_proj:
obj, cons = _helper_func_to_combine_objs_and_check_stuff(obj, cons)
else:
nonlinear_constraint = None
if not objective.built:
objective.build(verbose=verbose)There was a problem hiding this comment.
I am not sure about how to proceed with x_scale? Do people use it often? The constraint updater I have just uses the D for scaling.
YigitElma
commented
Feb 2, 2025
|
|
||
| # since D has changed, we need to update the ADinv | ||
| self._ADinv = (1 / Dnew)[unfixed_idx, None] * self._Ainv | ||
| self._D = Dnew |
There was a problem hiding this comment.
I also need to update the null space, like I did for Ainv. Previously, when I missed updating Ainv, test_bootstrap_consistency_current was failing.
Don't be misled by the 50% speed improvement in test_proximal_freeb_compute. It is because I made the constraint update part optional. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a
_eq_solve_objectiveproperty toProximalProjectionwhich will beLinearConstraintProjectionofForceBalanceand fixed boundary constraints.Updates the fixed parameter constraint targets of
_eq_solve_objective, and calculate particular solution without factorizationPasses
_eq_solve_objectivetoeq.perturbandeq.solveUpdates proximal-... benchmarks to include solve and perturb part. Previously, force balance was not solved since the previous x value was used. I slightly change the x using random number to include that part too
Resolves #1404