Skip to content
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

FixSumCoilCurrent objective #1130

Merged
merged 9 commits into from
Jul 17, 2024
Merged

FixSumCoilCurrent objective #1130

merged 9 commits into from
Jul 17, 2024

Conversation

ddudt
Copy link
Collaborator

@ddudt ddudt commented Jul 16, 2024

Resolves #1111
Resolves #1131

@dpanici I could use your help adding an example to the docs of how to set the target based on an equilibrium. There is the complication that coils with all positive/negative currents does not mean all of those currents are in the same physical direction, since the orientation of the coil geometries can differ. That is difficult to check in general, so we might need to simply warn the users to check their coil orientations themselves.

@ddudt ddudt requested a review from dpanici July 16, 2024 21:57
@ddudt ddudt added coil stuff relating to coils and coil optimization EZ-review labels Jul 16, 2024
@ddudt ddudt mentioned this pull request Jul 16, 2024
3 tasks
@dpanici
Copy link
Collaborator

dpanici commented Jul 16, 2024

Resolves #1111

@dpanici I could use your help adding an example to the docs of how to set the target based on an equilibrium. There is the complication that coils with all positive/negative currents does not mean all of those currents are in the same physical direction, since the orientation of the coil geometries can differ. That is difficult to check in general, so we might need to simply warn the users to check their coil orientations themselves.

coil orientations is definitely not something I had thought of before, good catch... maybe we can just make a note in the docstring of what convention we use (positive being CCW or CW if facing the phi direction). I will use the orientation from my regcoil stuff

Coil(s) that will be optimized to satisfy the Objective.
target : {float, ndarray}, optional
Target value(s) of the objective. Only used if bounds is None.
Must be broadcastable to Objective.dim_f. Default is FIXME.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can default it to whatever the initial sum is

@ddudt
Copy link
Collaborator Author

ddudt commented Jul 16, 2024

Resolves #1111
@dpanici I could use your help adding an example to the docs of how to set the target based on an equilibrium. There is the complication that coils with all positive/negative currents does not mean all of those currents are in the same physical direction, since the orientation of the coil geometries can differ. That is difficult to check in general, so we might need to simply warn the users to check their coil orientations themselves.

coil orientations is definitely not something I had thought of before, good catch... maybe we can just make a note in the docstring of what convention we use (positive being CCW or CW if facing the phi direction). I will use the orientation from my regcoil stuff

I don't even think we have a set convention for the sign of the current. Positive current means it is flowing in the direction of increasing curve parameter s, but that could be different depending on the curve type and orientation.

I think we should give an example showing how to set the target assuming all of the coil currents are flowing up/down through the center of the torus (or something like that). And then say it is the responsibility of the user to check the orientation of their coils to match that.

@dpanici
Copy link
Collaborator

dpanici commented Jul 16, 2024

Resolves #1111
@dpanici I could use your help adding an example to the docs of how to set the target based on an equilibrium. There is the complication that coils with all positive/negative currents does not mean all of those currents are in the same physical direction, since the orientation of the coil geometries can differ. That is difficult to check in general, so we might need to simply warn the users to check their coil orientations themselves.

coil orientations is definitely not something I had thought of before, good catch... maybe we can just make a note in the docstring of what convention we use (positive being CCW or CW if facing the phi direction). I will use the orientation from my regcoil stuff

I don't even think we have a set convention for the sign of the current. Positive current means it is flowing in the direction of increasing curve parameter s, but that could be different depending on the curve type and orientation.

I think we should give an example showing how to set the target assuming all of the coil currents are flowing up/down through the center of the torus (or something like that). And then say it is the responsibility of the user to check the orientation of their coils to match that.

There is an implicit orientation from the equilibrium Psi, so if Psi is positive then I think that means that B_zeta is positive and then G (which is a line integral of B_zeta around zeta which is equal to the net enclosed net poloidal current inside of the torus hole up to a factor of 2pi/mu0) is positive. That implies that coil currents flowing upwards through the torus hole are positive (I believe)

@dpanici
Copy link
Collaborator

dpanici commented Jul 16, 2024

I think in general we can leave this onus onto the user, our objective here just fixes the coil currents, if you want to use it to do something physcially meaningful, you have to know what that means (i.e. that the coils must link poloidally the torus, that there is a sign convention). This objective could also be used to just constrain the coil current sum for some other engineering reason (like if they are in parallel or something and the input feed has a limit of how much current it can take), so we don't need to go overboard on warnings inside this objective about orientations.

We could update the tutorial about this but I'd rather not before the release, we already changed so much in it.

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.29%. Comparing base (a02cf04) to head (d59f2b5).
Report is 1954 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1130   +/-   ##
=======================================
  Coverage   95.28%   95.29%           
=======================================
  Files          87       87           
  Lines       21694    21712   +18     
=======================================
+ Hits        20672    20690   +18     
  Misses       1022     1022           
Files with missing lines Coverage Δ
desc/objectives/__init__.py 100.00% <ø> (ø)
desc/objectives/linear_objectives.py 96.50% <100.00%> (+0.10%) ⬆️

... and 2 files with indirect coverage changes

docs/api.rst Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Jul 17, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     -0.08 +/- 7.45     | -4.00e-04 +/- 3.91e-02 |  5.24e-01 +/- 3.6e-02  |  5.25e-01 +/- 1.4e-02  |
 test_build_transform_fft_midres         |     -0.04 +/- 2.96     | -2.14e-04 +/- 1.80e-02 |  6.07e-01 +/- 8.5e-03  |  6.08e-01 +/- 1.6e-02  |
 test_build_transform_fft_highres        |     +0.67 +/- 4.91     | +6.67e-03 +/- 4.89e-02 |  1.00e+00 +/- 2.4e-02  |  9.94e-01 +/- 4.3e-02  |
 test_equilibrium_init_lowres            |     +0.02 +/- 1.35     | +7.84e-04 +/- 5.12e-02 |  3.78e+00 +/- 4.1e-02  |  3.78e+00 +/- 3.0e-02  |
 test_equilibrium_init_medres            |     -0.25 +/- 2.45     | -1.06e-02 +/- 1.04e-01 |  4.26e+00 +/- 6.2e-02  |  4.27e+00 +/- 8.4e-02  |
 test_equilibrium_init_highres           |     +0.17 +/- 0.85     | +9.86e-03 +/- 4.83e-02 |  5.67e+00 +/- 2.4e-02  |  5.66e+00 +/- 4.2e-02  |
 test_objective_compile_dshape_current   |     +0.76 +/- 1.27     | +2.92e-02 +/- 4.87e-02 |  3.86e+00 +/- 3.4e-02  |  3.83e+00 +/- 3.5e-02  |
 test_objective_compile_atf              |     -0.53 +/- 1.54     | -4.47e-02 +/- 1.28e-01 |  8.32e+00 +/- 9.1e-02  |  8.36e+00 +/- 9.0e-02  |
 test_objective_compute_dshape_current   |     +0.68 +/- 3.80     | +8.50e-06 +/- 4.73e-05 |  1.25e-03 +/- 3.1e-05  |  1.24e-03 +/- 3.6e-05  |
 test_objective_compute_atf              |     +1.26 +/- 4.69     | +5.33e-05 +/- 1.99e-04 |  4.29e-03 +/- 1.8e-04  |  4.23e-03 +/- 8.7e-05  |
 test_objective_jac_dshape_current       |     -2.88 +/- 14.67    | -1.09e-03 +/- 5.54e-03 |  3.67e-02 +/- 2.7e-03  |  3.77e-02 +/- 4.8e-03  |
 test_objective_jac_atf                  |     -0.36 +/- 3.04     | -6.86e-03 +/- 5.72e-02 |  1.87e+00 +/- 4.9e-02  |  1.88e+00 +/- 2.9e-02  |
 test_perturb_1                          |     +0.80 +/- 0.60     | +1.06e-01 +/- 7.95e-02 |  1.34e+01 +/- 5.9e-02  |  1.33e+01 +/- 5.4e-02  |
 test_perturb_2                          |     -0.07 +/- 0.63     | -1.22e-02 +/- 1.15e-01 |  1.84e+01 +/- 9.3e-02  |  1.84e+01 +/- 6.8e-02  |
 test_proximal_jac_atf                   |     +0.12 +/- 1.24     | +8.67e-03 +/- 9.01e-02 |  7.30e+00 +/- 6.1e-02  |  7.29e+00 +/- 6.6e-02  |
 test_proximal_freeb_compute             |     -0.33 +/- 0.92     | -5.95e-04 +/- 1.66e-03 |  1.80e-01 +/- 8.8e-04  |  1.81e-01 +/- 1.4e-03  |
 test_proximal_freeb_jac                 |     +0.47 +/- 0.98     | +3.47e-02 +/- 7.24e-02 |  7.43e+00 +/- 4.7e-02  |  7.39e+00 +/- 5.5e-02  |
 test_solve_fixed_iter                   |     +0.60 +/- 8.66     | +9.08e-02 +/- 1.30e+00 |  1.51e+01 +/- 9.9e-01  |  1.51e+01 +/- 8.5e-01  |

@ddudt ddudt requested a review from f0uriest July 17, 2024 16:00
@ddudt ddudt merged commit eded83f into master Jul 17, 2024
18 checks passed
@ddudt ddudt deleted the dd/sum-coil-currents branch July 17, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coil stuff relating to coils and coil optimization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test new functionality for LinearObjectiveFromUser Make Linear Objective to Constrain Sum of Coil Currents
4 participants