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

omnigenity #535

Merged
merged 227 commits into from
Mar 18, 2024
Merged

omnigenity #535

merged 227 commits into from
Mar 18, 2024

Conversation

ddudt
Copy link
Collaborator

@ddudt ddudt commented Jun 14, 2023

Adds omnigenity compute quantities and objective function.

This fits with the existing API, where all compute arguments have to be attributes of the Equilibrium class. Those new attributes are:

  • well_l = the Chebyshev/monotonic spline points parameterizing $B(\rho,\nu)$
  • omni_lmn = the Cheyshev-Fourier coefficients ($x_{lmn}$ in my paper) parameterizing the coordinate mapping $h(\theta_B,\zeta_B)=h(\alpha,\eta)$
  • associated Bases and resolution attributes for the above parameters

I don't love these variable names and am open to other suggestions.

There are several other miscellaneous things that got lumped in with this PR (sorry). Otherwise it should be a straightforward implementation of a new physics objective. See the optimization scripts in publications/dudt2023/ for examples of how to use the new code.

Resolves #794
Resolves #776

ddudt added 30 commits April 15, 2023 13:24
- Adds most of code for new Omnigenity compute and objective functions.
- Needs more changes to work with optimization API.
- Misc other edits.
- Renamed omni_mn to omni_lmn.
- Changed basis of omni params from DoubleFourierSeries to FourierZernikeBasis.
- Added StraightBmaxContour linear constraint instead of applying boundary condition in compute function.
- Add a power series to the basis for the well shape parameters, so there can be a different well on each surface.
- Renamed some of the variables and classes.
Correction for NFP>1. Tested on QI example and looks decent.
This reverts commit c11cf93.
@ddudt
Copy link
Collaborator Author

ddudt commented Mar 6, 2024

  • Are the plot scripts for your paper updated or are they still just using the old version of the code? If the latter then should edit the description of the PR with that info (and with the new variable names)
  • Try to merge the plot_boozer_surface and plot_omnigenous_field functions as the only difference is how the |B| at points is obtained and the contour plot function used, there is duplicated code there, for instance when you plot the fieldlines you use the same logic as plot_boozer_surface
  • A test for having a field fixed would be good as there's some nontrivial logic in the objective for that case
  • specify that grids must be non symmetric in the docstring of Omnigenity objective
  • Either be more specific in how the B_lm array should be flattened in the dosctring of OmnigenousField or allow the shaped array to be passed in instead and do all flattening internally
  • specify what coordiantes the Grid is expected to be in for both OmnigenousField and the Omnigenity objective (and specifically that theta is treated as eta and zeta as alpha)
  • some other miscellaneous dosctring improvements for OmnigenousField, including adding some math or referencing your paper and any discrepancies in variable names btwn the paper and the final version of the code here
  • Add a check that none of the spline knot values are negative in the build method of Omnigenity (or even just in the OmnigenousField itself when those parameters are set)
  • Be more specific in the docstring of what eta_weight is

These changes were all made, including updating the plotting script to generate the figures from the paper with the latest version of the code.

f0uriest
f0uriest previously approved these changes Mar 7, 2024
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.

My last comments ( probably)

  • can you add the paper reference in the omniigenous field class?
  • make an issue to fix the reverse mode jacobian for the omnigenity obj
  • clarify grid node coordinates -> is field_grid actually coordinates in (rho, eta, alpha) i.e. if I pass in a grid with theta points going from 0 to 2pi is that wrong, am I actually supposed to pass in a grid with points fro -pi/2 to pi/2 for the theta nodes? The docstring still seemed confusing to me as you use "mapping" but mapping to me implies doing a transformation

@f0uriest
Copy link
Member

Also make sure to change the base branch for #631 after merging this but before deleting this branch

f0uriest
f0uriest previously approved these changes Mar 15, 2024
dpanici
dpanici previously approved these changes Mar 16, 2024
@daniel-dudt daniel-dudt dismissed stale reviews from dpanici and f0uriest via 880de18 March 17, 2024 21:13
@ddudt ddudt requested review from f0uriest and dpanici March 17, 2024 21:30
@ddudt ddudt merged commit 073efec into master Mar 18, 2024
16 of 17 checks passed
@ddudt ddudt deleted the dd/omnigenity branch March 18, 2024 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tutorial: Optimization with Multiple Objects Tutorial: Omnigenity optimization
5 participants