-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add qfm surface functionality #1264
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1264 +/- ##
==========================================
- Coverage 95.44% 95.44% -0.01%
==========================================
Files 95 95
Lines 23423 23437 +14
==========================================
+ Hits 22357 22369 +12
- Misses 1066 1068 +2
|
| benchmark_name | dt(%) | dt(s) | t_new(s) | t_old(s) |
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
test_build_transform_fft_lowres | -1.86 +/- 4.86 | -9.98e-03 +/- 2.61e-02 | 5.26e-01 +/- 1.0e-02 | 5.36e-01 +/- 2.4e-02 |
test_equilibrium_init_medres | +0.08 +/- 2.61 | +3.55e-03 +/- 1.10e-01 | 4.21e+00 +/- 1.1e-01 | 4.21e+00 +/- 2.3e-02 |
test_equilibrium_init_highres | -0.03 +/- 4.05 | -1.63e-03 +/- 2.25e-01 | 5.57e+00 +/- 2.0e-01 | 5.57e+00 +/- 1.0e-01 |
test_objective_compile_dshape_current | -0.56 +/- 6.06 | -2.21e-02 +/- 2.38e-01 | 3.90e+00 +/- 2.4e-01 | 3.92e+00 +/- 2.4e-02 |
test_objective_compute_dshape_current | +1.40 +/- 1.56 | +5.04e-05 +/- 5.62e-05 | 3.66e-03 +/- 4.9e-05 | 3.61e-03 +/- 2.7e-05 |
test_objective_jac_dshape_current | +3.53 +/- 6.88 | +1.44e-03 +/- 2.82e-03 | 4.24e-02 +/- 2.4e-03 | 4.09e-02 +/- 1.5e-03 |
test_perturb_2 | +2.49 +/- 2.98 | +4.38e-01 +/- 5.22e-01 | 1.80e+01 +/- 4.3e-01 | 1.76e+01 +/- 3.0e-01 |
test_proximal_freeb_jac | +0.35 +/- 1.57 | +2.66e-02 +/- 1.18e-01 | 7.56e+00 +/- 4.8e-02 | 7.54e+00 +/- 1.1e-01 |
test_solve_fixed_iter | +0.07 +/- 60.15 | +3.37e-03 +/- 3.04e+00 | 5.05e+00 +/- 2.1e+00 | 5.05e+00 +/- 2.2e+00 |
test_build_transform_fft_midres | -6.04 +/- 2.90 | -4.00e-02 +/- 1.92e-02 | 6.23e-01 +/- 1.6e-02 | 6.63e-01 +/- 1.0e-02 |
test_build_transform_fft_highres | -3.85 +/- 3.23 | -4.11e-02 +/- 3.44e-02 | 1.03e+00 +/- 2.7e-02 | 1.07e+00 +/- 2.1e-02 |
test_equilibrium_init_lowres | -4.94 +/- 2.92 | -2.14e-01 +/- 1.27e-01 | 4.13e+00 +/- 1.2e-01 | 4.34e+00 +/- 5.2e-02 |
test_objective_compile_atf | -1.72 +/- 3.54 | -1.45e-01 +/- 2.99e-01 | 8.28e+00 +/- 1.3e-01 | 8.43e+00 +/- 2.7e-01 |
test_objective_compute_atf | +1.74 +/- 3.38 | +1.93e-04 +/- 3.76e-04 | 1.13e-02 +/- 1.9e-04 | 1.11e-02 +/- 3.2e-04 |
test_objective_jac_atf | -0.50 +/- 3.47 | -1.01e-02 +/- 6.99e-02 | 2.01e+00 +/- 5.2e-02 | 2.02e+00 +/- 4.7e-02 |
test_perturb_1 | +0.21 +/- 4.02 | +2.86e-02 +/- 5.48e-01 | 1.37e+01 +/- 1.9e-01 | 1.36e+01 +/- 5.1e-01 |
test_proximal_jac_atf | +0.24 +/- 0.83 | +2.00e-02 +/- 6.94e-02 | 8.36e+00 +/- 4.7e-02 | 8.34e+00 +/- 5.1e-02 |
test_proximal_freeb_compute | +1.49 +/- 1.00 | +2.78e-03 +/- 1.87e-03 | 1.90e-01 +/- 1.2e-03 | 1.87e-01 +/- 1.5e-03 | |
Equilibrium upon whose surface the normal field error will be minimized. | ||
The equilibrium is kept fixed during the optimization with this objective. | ||
eq : Equilibrium or FourierRZToroidalSurface | ||
Equilibrium (or QFM surface) upon whose surface the normal field error |
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.
should maybe add a note to use BoundaryError or VacuumBoundaryError for free boundary / single stage optimization
self._vacuum = vacuum or qfm_surface | ||
self._qfm_surface = qfm_surface | ||
errorif( | ||
qfm_surface and hasattr(eq, "L_lmn"), |
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.
would prefer an actual instance check here instead of relying on attributes to be/not be there
@@ -1182,8 +1198,23 @@ def __init__( | |||
self._eq = eq | |||
self._field = field | |||
self._field_grid = field_grid | |||
self._vacuum = vacuum | |||
things = [field] | |||
self._vacuum = vacuum or qfm_surface |
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.
might be better to throw an error here if they're incompatible?
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.
The thing is, by default vacuum=False
, so an error would be thrown if qfm_surface=True
is passed in.
I guess that is fine if we want users to have to be verbose about asking for qfm surface and specifying they are for a vacuum field
@@ -1282,14 +1323,25 @@ def compute(self, field_params, constants=None): | |||
""" | |||
if constants is None: | |||
constants = self.constants | |||
|
|||
field_params = params_1 if not self._qfm_surface else params_2 |
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.
don't you also need to check if the field is fixed? in which case the only DOFs will be the surface?
x = jnp.array([eval_data["R"], eval_data["phi"], eval_data["Z"]]).T | ||
|
||
# B_ext is not pre-computed because field is not fixed | ||
if not self._qfm_surface: |
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.
minor point but I think its clearer if this is re-ordered to be
if qfm_surface:
...
else:
...
field_params = params_1 if not self._qfm_surface else params_2 | ||
surf_params = params_2 if not self._qfm_surface else params_1 | ||
|
||
if not self._qfm_surface: |
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.
see above
make a new objective for this specifically |
Resolve #1162
ToroidalFlux
to acceptFourierRZToroidalSurface
so that we can specify the toroidal flux through the QFM (we can do the vector potential method on the surface just fine so this should be easy, and there is no plasma current to consider either)