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

ko/coils quad flux #726

Merged
merged 85 commits into from
Apr 6, 2024
Merged

ko/coils quad flux #726

merged 85 commits into from
Apr 6, 2024

Conversation

kianorr
Copy link
Collaborator

@kianorr kianorr commented Oct 25, 2023

Adds quadratic flux objective function.

Resolves #901

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions
Copy link
Contributor

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     -6.03 +/- 3.20     | -1.48e-03 +/- 7.86e-04 |  2.31e-02 +/- 4.4e-04  |  2.45e-02 +/- 6.5e-04  |
+test_build_transform_fft_midres         |     -4.28 +/- 0.93     | -5.92e-03 +/- 1.28e-03 |  1.32e-01 +/- 8.4e-04  |  1.38e-01 +/- 9.7e-04  |
+test_build_transform_fft_highres        |     -3.57 +/- 1.03     | -2.26e-02 +/- 6.54e-03 |  6.10e-01 +/- 3.4e-03  |  6.33e-01 +/- 5.6e-03  |
 test_equilibrium_init_lowres            |     -1.60 +/- 2.45     | -1.64e-02 +/- 2.53e-02 |  1.01e+00 +/- 1.6e-02  |  1.03e+00 +/- 1.9e-02  |
 test_equilibrium_init_medres            |     -2.65 +/- 2.12     | -4.08e-02 +/- 3.27e-02 |  1.50e+00 +/- 2.8e-02  |  1.54e+00 +/- 1.6e-02  |
 test_equilibrium_init_highres           |     -2.59 +/- 1.01     | -9.51e-02 +/- 3.72e-02 |  3.57e+00 +/- 2.7e-02  |  3.67e+00 +/- 2.6e-02  |
 test_objective_compile_dshape_current   |     -0.55 +/- 5.32     | -3.58e-02 +/- 3.48e-01 |  6.50e+00 +/- 1.9e-01  |  6.54e+00 +/- 2.9e-01  |
 test_objective_compile_atf              |     +9.19 +/- 5.32     | +1.72e+00 +/- 9.93e-01 |  2.04e+01 +/- 6.0e-01  |  1.87e+01 +/- 7.9e-01  |
 test_objective_compute_dshape_current   |     -6.75 +/- 4.34     | -2.93e-04 +/- 1.88e-04 |  4.04e-03 +/- 1.0e-04  |  4.33e-03 +/- 1.6e-04  |
 test_objective_compute_atf              |     +3.86 +/- 9.26     | +5.65e-04 +/- 1.36e-03 |  1.52e-02 +/- 1.2e-03  |  1.46e-02 +/- 6.8e-04  |
 test_objective_jac_dshape_current       |    +16.23 +/- 12.06    | +2.58e-02 +/- 1.91e-02 |  1.85e-01 +/- 1.8e-02  |  1.59e-01 +/- 7.0e-03  |
-test_objective_jac_atf                  |    +16.91 +/- 2.06     | +1.35e+00 +/- 1.65e-01 |  9.33e+00 +/- 1.4e-01  |  7.98e+00 +/- 7.8e-02  |
 test_perturb_1                          |    +35.04 +/- 14.51    | +3.77e+00 +/- 1.56e+00 |  1.45e+01 +/- 1.2e+00  |  1.08e+01 +/- 1.0e+00  |
-test_perturb_2                          |    +17.19 +/- 5.28     | +3.54e+00 +/- 1.09e+00 |  2.42e+01 +/- 6.0e-01  |  2.06e+01 +/- 9.1e-01  |

@dpanici dpanici changed the base branch from master to rc/freeb October 25, 2023 19:49
Copy link
Collaborator

@YigitElma YigitElma left a comment

Choose a reason for hiding this comment

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

Review for APC524 homework, pull request review part.

@kianorr kianorr marked this pull request as ready for review January 3, 2024 03:53
@kianorr
Copy link
Collaborator Author

kianorr commented Jan 3, 2024

Not sure if having QuadraticFlux in _free_boundary.py is appropriate but I'm not sure where you all would prefer to put it

Copy link
Collaborator

@dpanici dpanici left a comment

Choose a reason for hiding this comment

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

  • Add docstring to the compute function
  • Add an eq_fixed option to this so that you can only optiimze an external field given a fixed equilibrium
  • Add a test where you optimize a field with quadratic flux, something like this might work I think (since the equilibrum is axisymmetric the optimizer should put the non-axisymmetric part of the external field to zero, i.e. zero out the Phi_mn, though if this does not work lmk)
eq = Equilibrium(M=2,L=2)
eq.solve(verbose=0)
surf = eq.surface.copy()
surf.R_lmn[surf.R_basis.get_idx(M=1,N=0)] = 2
surf.Z_lmn[surf.Z_basis.get_idx(M=-1,N=0)] = 2

field = FourierCurrentPotentialField.from_surface(surface=surf, Phi_mn=np.array([1e4]),modes_Phi=np.array([[1,4]])),G=1e4)

constraints=(FixParameter(eq), # this line is unneeded when eq_fixed is implemented
                     FixParameter(field,["I","G","R_lmn","Z_lmn"]))
optimizer = Optimizer("lsq-exact")
obj = ObjectiveFunction(QuadraticFlux(ext_field=field,eq=eq))
(eq,field) = optimizer.optimize((eq,field),objective=obj,constraints=constraints,ftol=1e-15,xtol=1e-15)
np.testing.assert_allclose(field.Phi_mn,0,atol=1e-14)

dpanici
dpanici previously approved these changes Apr 4, 2024
dpanici
dpanici previously approved these changes Apr 4, 2024
@dpanici dpanici requested a review from f0uriest April 4, 2024 18:20
f0uriest
f0uriest previously approved these changes Apr 4, 2024
f0uriest
f0uriest previously approved these changes Apr 5, 2024
dpanici
dpanici previously approved these changes Apr 5, 2024
Copy link
Collaborator

@ddudt ddudt left a comment

Choose a reason for hiding this comment

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

Since the QuadraticFlux objective assumes the Equilibrium is held fixed during the optimization, I think we need to add a check for that. Like if QuadraticFlux is in optimizer.objective, then we should throw an error if eq is in optimizer.things.

np.testing.assert_allclose(f, 0, rtol=1e-14, atol=1e-14)

# test non-axisymmetric surface
eq = desc.examples.get("precise_QA", "all")[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use the initial equilibrium from this solve and not the "real" precise QA equilibrium?

@daniel-dudt daniel-dudt dismissed stale reviews from dpanici and f0uriest via 0d84c4e April 5, 2024 20:52
@f0uriest
Copy link
Member

f0uriest commented Apr 5, 2024

Since the QuadraticFlux objective assumes the Equilibrium is held fixed during the optimization, I think we need to add a check for that. Like if QuadraticFlux is in optimizer.objective, then we should throw an error if eq is in optimizer.things.

This is a good idea, I'll add this.

@f0uriest f0uriest requested review from ddudt and dpanici April 5, 2024 21:07
@f0uriest f0uriest merged commit 0a9b1ab into master Apr 6, 2024
18 checks passed
@f0uriest f0uriest deleted the ko/coils_quad_flux branch April 6, 2024 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hackathon Stuff to work on during hackathon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Objective function for Bnormal
6 participants