-
Notifications
You must be signed in to change notification settings - Fork 26
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 documentation for default grid and target/bounds for objectives #988
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #988 +/- ##
=======================================
Coverage 95.34% 95.34%
=======================================
Files 87 87
Lines 21778 21778
=======================================
Hits 20765 20765
Misses 1013 1013
|
| benchmark_name | dt(%) | dt(s) | t_new(s) | t_old(s) |
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
test_build_transform_fft_lowres | -1.48 +/- 4.34 | -2.73e-02 +/- 8.03e-02 | 1.82e+00 +/- 5.1e-02 | 1.85e+00 +/- 6.2e-02 |
test_build_transform_fft_midres | -3.75 +/- 3.98 | -7.71e-02 +/- 8.18e-02 | 1.98e+00 +/- 4.5e-02 | 2.06e+00 +/- 6.8e-02 |
test_build_transform_fft_highres | -3.68 +/- 3.47 | -9.18e-02 +/- 8.66e-02 | 2.41e+00 +/- 7.2e-02 | 2.50e+00 +/- 4.8e-02 |
test_equilibrium_init_lowres | -3.97 +/- 2.21 | -4.04e-01 +/- 2.25e-01 | 9.78e+00 +/- 1.8e-01 | 1.02e+01 +/- 1.3e-01 |
test_equilibrium_init_medres | -3.53 +/- 3.55 | -3.76e-01 +/- 3.79e-01 | 1.03e+01 +/- 2.9e-01 | 1.07e+01 +/- 2.4e-01 |
test_equilibrium_init_highres | -0.82 +/- 2.01 | -9.92e-02 +/- 2.42e-01 | 1.20e+01 +/- 1.5e-01 | 1.21e+01 +/- 1.9e-01 |
test_objective_compile_dshape_current | -2.72 +/- 10.07 | -9.93e-02 +/- 3.68e-01 | 3.55e+00 +/- 2.8e-01 | 3.65e+00 +/- 2.4e-01 |
test_objective_compile_atf | -1.32 +/- 2.95 | -9.57e-02 +/- 2.14e-01 | 7.15e+00 +/- 1.4e-01 | 7.25e+00 +/- 1.6e-01 |
test_objective_compute_dshape_current | -0.18 +/- 2.39 | -7.02e-06 +/- 9.44e-05 | 3.94e-03 +/- 5.4e-05 | 3.95e-03 +/- 7.8e-05 |
test_objective_compute_atf | -1.91 +/- 2.36 | -3.45e-04 +/- 4.26e-04 | 1.77e-02 +/- 2.6e-04 | 1.81e-02 +/- 3.3e-04 |
test_objective_jac_dshape_current | +2.71 +/- 6.25 | +1.14e-03 +/- 2.62e-03 | 4.30e-02 +/- 2.1e-03 | 4.19e-02 +/- 1.6e-03 |
test_objective_jac_atf | +2.20 +/- 3.48 | +4.10e-02 +/- 6.50e-02 | 1.91e+00 +/- 4.4e-02 | 1.87e+00 +/- 4.8e-02 |
test_perturb_1 | +0.21 +/- 5.08 | +3.09e-02 +/- 7.38e-01 | 1.46e+01 +/- 4.9e-01 | 1.45e+01 +/- 5.5e-01 |
test_perturb_2 | -1.34 +/- 3.23 | -2.64e-01 +/- 6.36e-01 | 1.94e+01 +/- 2.7e-01 | 1.97e+01 +/- 5.8e-01 |
test_proximal_jac_atf | -1.07 +/- 1.51 | -7.67e-02 +/- 1.08e-01 | 7.09e+00 +/- 6.3e-02 | 7.16e+00 +/- 8.8e-02 |
test_proximal_freeb_compute | +0.34 +/- 0.90 | +4.42e-04 +/- 1.16e-03 | 1.29e-01 +/- 8.1e-04 | 1.29e-01 +/- 8.4e-04 |
test_proximal_freeb_jac | +1.14 +/- 1.90 | +8.15e-02 +/- 1.36e-01 | 7.25e+00 +/- 1.2e-01 | 7.16e+00 +/- 6.9e-02 | |
@@ -255,10 +255,11 @@ class CoilLength(_CoilObjective): | |||
target : float, ndarray, optional | |||
Target value(s) of the objective. Only used if bounds is None. | |||
Must be broadcastable to Objective.dim_f. If array, it has to | |||
be flattened according to the number of inputs. | |||
be flattened according to the number of inputs. Defaults to ``target=2*np.pi``. |
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.
Shouldn't this default depend on the diameter of the coil. What if the minor radius of the last surface is 10 ?
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.
We could maybe try to make better default values, but it should be in another PR. This one is just about documenting what is currently there.
@@ -527,10 +536,11 @@ class PlasmaVesselDistance(_Objective): | |||
Bounding surface to penalize distance to. | |||
target : {float, ndarray}, optional | |||
Target value(s) of the objective. Only used if bounds is None. | |||
Must be broadcastable to Objective.dim_f. | |||
Must be broadcastable to Objective.dim_f. Defaults to ``bounds=(1,np.inf)``. |
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.
Same as my previous questions. Can we make defaults smarter?
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.
This one is fairly sensible I think, it's just saying we want the vessel to be at least 1m away from the plasma. There will always be a tradeoff between how complicated the default is vs how useful it is. In 99% of cases, the user should be setting a target/bounds themselves anyways.
Resolves #975