-
Notifications
You must be signed in to change notification settings - Fork 41
Add option to choose Proximal inversion method #2031
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
base: master
Are you sure you want to change the base?
Changes from all commits
7d7537e
5314810
64c7a7f
71686cf
52a08a4
6d39253
1dde9c2
7fc4383
7dc09f2
71c10ee
0eb8369
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
|
|
||
| import numpy as np | ||
|
|
||
| from desc.backend import jit, jnp, put | ||
| from desc.backend import jit, jnp, put, qr | ||
| from desc.batching import batched_vectorize | ||
| from desc.objectives import ( | ||
| BoundaryRSelfConsistency, | ||
|
|
@@ -21,7 +21,7 @@ | |
| ) | ||
| from desc.utils import Timer, errorif, get_instance, setdefault | ||
|
|
||
| from .utils import f_where_x | ||
| from .utils import f_where_x, solve_triangular_regularized | ||
|
|
||
|
|
||
| class LinearConstraintProjection(ObjectiveFunction): | ||
|
|
@@ -572,6 +572,9 @@ | |
| perturb_options, solve_options : dict | ||
| dictionary of arguments passed to Equilibrium.perturb and Equilibrium.solve | ||
| during the projection step. | ||
| inv_method : str | ||
| Method to use for computing the pseudo-inverse of the constraint Jacobian. | ||
| Options are 'svd' (default) and 'qr'. | ||
| name : str | ||
| Name of the objective function. | ||
| """ | ||
|
|
@@ -583,14 +586,20 @@ | |
| eq, | ||
| perturb_options=None, | ||
| solve_options=None, | ||
| inv_method="qr", | ||
| name="ProximalProjection", | ||
| ): | ||
| assert isinstance(objective, ObjectiveFunction), ( | ||
| "objective should be instance of ObjectiveFunction." "" | ||
| ) | ||
| assert isinstance(constraint, ObjectiveFunction), ( | ||
| "constraint should be instance of ObjectiveFunction." "" | ||
| ) | ||
| assert isinstance( | ||
| objective, ObjectiveFunction | ||
| ), "objective should be instance of ObjectiveFunction." | ||
| assert isinstance( | ||
| constraint, ObjectiveFunction | ||
| ), "constraint should be instance of ObjectiveFunction." | ||
| assert inv_method in [ | ||
| "svd", | ||
| "qr", | ||
| "svd-reg", | ||
| ], f"inv_method should be either 'svd', 'svd-reg' or 'qr', got {inv_method}." | ||
| for con in constraint.objectives: | ||
| errorif( | ||
| not con._equilibrium, | ||
|
|
@@ -618,6 +627,7 @@ | |
| solve_options.setdefault("verbose", 0) | ||
| self._perturb_options = perturb_options | ||
| self._solve_options = solve_options | ||
| self._inv_method = inv_method | ||
| self._built = False | ||
| # don't want to compile this, just use the compiled objective and constraint | ||
| self._use_jit = False | ||
|
|
@@ -1262,6 +1272,7 @@ | |
| self._eq_solve_objective._feasible_tangents, | ||
| self._dxdc, | ||
| op, | ||
| inv_method=self._inv_method, | ||
| ) | ||
| # broadcasting against multiple things | ||
| dfdcs = [jnp.zeros(dim) for dim in self._dimc_per_thing] | ||
|
|
@@ -1310,8 +1321,10 @@ | |
| # define these helper functions that are stateless so we can safely jit them | ||
|
|
||
|
|
||
| @functools.partial(jit, static_argnames=["op"]) | ||
| def _proximal_jvp_f_pure(constraint, xf, constants, dc, eq_feasible_tangents, dxdc, op): | ||
| @functools.partial(jit, static_argnames=["op", "inv_method"]) | ||
| def _proximal_jvp_f_pure( | ||
| constraint, xf, constants, dc, eq_feasible_tangents, dxdc, op, inv_method="svd" | ||
| ): | ||
| # Note: This function is called by _get_tangent which is vectorized over v | ||
| # (v is called dc in this function). So, dc is expected to be 1D array | ||
| # of same size as full equilibrium state vector. This function returns a 1D array. | ||
|
|
@@ -1326,11 +1339,20 @@ | |
| # wrt all R_lmn coefficients that contribute to Rb_023. See BoundaryRSelfConsistency | ||
| # for the relation between Rb_lmn and R_lmn. | ||
| Fc = getattr(constraint, "jvp_" + op)(dxdc @ dc, xf, constants) | ||
| cutoff = jnp.finfo(Fxh.dtype).eps * max(Fxh.shape) | ||
| uf, sf, vtf = jnp.linalg.svd(Fxh, full_matrices=False) | ||
| sf += sf[-1] # add a tiny bit of regularization | ||
| sfi = jnp.where(sf < cutoff * sf[0], 0, 1 / sf) | ||
| return vtf.T @ (sfi * (uf.T @ Fc)) | ||
| if inv_method == "svd": | ||
| cutoff = jnp.finfo(Fxh.dtype).eps * max(Fxh.shape) | ||
| uf, sf, vtf = jnp.linalg.svd(Fxh, full_matrices=False) | ||
| sfi = jnp.where(sf < cutoff * sf[0], 0, 1 / sf) | ||
| return vtf.T @ (sfi * (uf.T @ Fc)) | ||
| elif inv_method == "svd-reg": # this option will be deleted | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why the comment on deleting the option?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was planning to remove it, but I will keep it at least for foreseable future |
||
| cutoff = jnp.finfo(Fxh.dtype).eps * max(Fxh.shape) | ||
| uf, sf, vtf = jnp.linalg.svd(Fxh, full_matrices=False) | ||
| sf += sf[-1] | ||
| sfi = jnp.where(sf < cutoff * sf[0], 0, 1 / sf) | ||
| return vtf.T @ (sfi * (uf.T @ Fc)) | ||
| elif inv_method == "qr": | ||
| Q, R = qr(Fxh, mode="economic") | ||
| return solve_triangular_regularized(R, Q.T @ Fc) | ||
|
|
||
|
|
||
| @functools.partial(jit, static_argnames=["op"]) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -601,12 +601,14 @@ def _maybe_wrap_nonlinear_constraints( | |
| if wrapper is not None and wrapper.lower() in ["prox", "proximal"]: | ||
| perturb_options = options.pop("perturb_options", {}) | ||
| solve_options = options.pop("solve_options", {}) | ||
| inv_method = options.pop("prox_inv_method", "qr") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to see more testing on a wide class of problems before we change the default.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed, I will try it on some trickier free boundary problems I have |
||
| objective = ProximalProjection( | ||
| objective, | ||
| constraint=_combine_constraints(nonlinear_constraints), | ||
| perturb_options=perturb_options, | ||
| solve_options=solve_options, | ||
| eq=eq, | ||
| inv_method=inv_method, | ||
| ) | ||
| nonlinear_constraints = () | ||
| return objective, nonlinear_constraints | ||
|
|
||
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -265,30 +265,13 @@ def test_qh_optimization(): | |
| eq = Equilibrium(M=5, N=5, Psi=0.04, surface=surf) | ||
| eq.solve(verbose=3) | ||
|
|
||
| eq1 = run_qh_step(0, eq) | ||
| eq1 = run_qh_step(2, eq) | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should check the notebooks to see if anything is changed with this
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this test doesn't work with the new option that suggests that there may be something wrong. We should figure out what the actual issue is rather than changing the test. (note that this is such low resolution that it would already converge without the fourier continuation, but we want to test that part as well) |
||
|
|
||
| obj = QuasisymmetryBoozer(helicity=(1, eq1.NFP), eq=eq1) | ||
| obj.build() | ||
| B_asym = obj.compute(*obj.xs(eq1)) | ||
|
|
||
| np.testing.assert_array_less(np.abs(B_asym).max(), 1e-1) | ||
| np.testing.assert_array_less(eq1.compute("a_major/a_minor")["a_major/a_minor"], 5) | ||
|
|
||
| eq2 = run_qh_step(1, eq1) | ||
|
|
||
| obj = QuasisymmetryBoozer(helicity=(1, eq2.NFP), eq=eq2) | ||
| obj.build() | ||
| B_asym = obj.compute(*obj.xs(eq2)) | ||
| np.testing.assert_array_less(np.abs(B_asym).max(), 1e-2) | ||
| np.testing.assert_array_less(eq2.compute("a_major/a_minor")["a_major/a_minor"], 5) | ||
|
|
||
| eq3 = run_qh_step(2, eq2) | ||
|
|
||
| obj = QuasisymmetryBoozer(helicity=(1, eq3.NFP), eq=eq3) | ||
| obj.build() | ||
| B_asym = obj.compute(*obj.xs(eq3)) | ||
| np.testing.assert_array_less(np.abs(B_asym).max(), 2e-3) | ||
| np.testing.assert_array_less(eq3.compute("a_major/a_minor")["a_major/a_minor"], 5) | ||
| np.testing.assert_array_less(eq1.compute("a_major/a_minor")["a_major/a_minor"], 5) | ||
|
|
||
|
|
||
| @pytest.mark.regression | ||
|
|
||
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.
these example cases are super low res, so I wouldn't take that as representative of the true behavior