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

TERPSICHORE #1046

Open
wants to merge 100 commits into
base: master
Choose a base branch
from
Open

TERPSICHORE #1046

wants to merge 100 commits into from

Conversation

ddudt
Copy link
Collaborator

@ddudt ddudt commented Jun 7, 2024

New objective function to wrap the TERPSICHORE stability code

@ddudt ddudt added interface New feature or request to make the code more usable or compatibility with another code funtionality New feature or request to do things the code can't do now. objectives Adding or improving objective functions labels Jun 7, 2024
@ddudt ddudt assigned ddudt and mfmart Jun 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.

some clarifying comments requested, and fix the docs formatting as each subheader seems to show as its own link

image

This guide begins with a simple example of how to use a custom function with the
``ExternalObjective``. It then uses the ``TERPSICHORE`` objective as a more realistic
example, which can be used as a template for wrapping other codes. Finally, an example
script is shown for how to run the TERPSICHORE optimization with multiprocessing.
Copy link
Collaborator

Choose a reason for hiding this comment

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

add a note that one must obtain the TERPSICHORE source themselves and that DESC does not have the code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

either add note here or later in this doc

@rahulgaur104
Copy link
Collaborator

Should there be a test tto check if the interface works? Maybe have a dummy function instead of the actual TERPSICHORE solver?

@@ -707,7 +707,7 @@ class TERPSICHORE(ExternalObjective):
plasma. If TERPSICHORE fails to run, try decreasing this parameter. Default = 2.
deltaJp : float, optional
Resonance detuning parameter to resolve parallel current density singularities.
Default = 0.5.
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there any intuition to be gained from understanding this default? like why did you lower it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TERPSICHORE documentation recommends a value in the range 1E-4 to 4E-2, I just accidentally had the default way too high.

The inputs awall and deltaJp both artificially stabilize the plasma but make the numerics better, so typically you need to do a convergence scan in both parameters and the "best" value is problem dependent.

--------------------

The ``ExternalObjective`` class allows external codes to be used within a DESC
optimization. This takes a user-supplied function that does not need not be JAX
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
optimization. This takes a user-supplied function that does not need not be JAX
optimization. This takes a user-supplied function that does not need to be JAX

language besides Python, or cannot be rewritten with JAX for some other reason. If the
user function can be made JAX transformable, it is recommended to use the
``ObjectiveFromUser`` class instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be good to restate desc/experimental/README here

code TERPSICHORE, which is written in FORTRAN. It summarizes how the external function
was written to call TERPSICHORE from DESC, and can be used as a template for wrapping
other codes. The code shown here is abbreviated and slightly modified for brevity, but
the full source code can be found in ``desc/experimental/_terpsichore.py``.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree with Dario, this sentence sounds like we have the source code of TERPSICHORE (or this could be my ignorance)

@@ -82,6 +80,7 @@
dev_guide/notebooks/grid.ipynb
dev_guide/adding_compute_funs.rst
dev_guide/adding_objectives.rst
dev_guide/external_objectives.rst
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
dev_guide/external_objectives.rst
external_objectives.rst

or move it to dev_guide/

ddudt added a commit that referenced this pull request Feb 17, 2025
Creates an abstract base class for wrapping external codes with finite
differences, like GX, TERPSICHORE, etc.

TODO:

- [x] vectorize
- [x] reverse mode gradient (hard-coded to be forward mode, because
using forward finite difference)
- [x] tests
- [x] tutorial (will be added by #1046)
Base automatically changed from dd/external to master February 17, 2025 15:10
Copy link

codecov bot commented Feb 17, 2025

Codecov Report

Attention: Patch coverage is 0% with 385 lines in your changes missing coverage. Please review.

Project coverage is 94.32%. Comparing base (5492090) to head (a8cf7d0).

Files with missing lines Patch % Lines
desc/experimental/_terpsichore.py 0.00% 384 Missing ⚠️
desc/experimental/__init__.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1046      +/-   ##
==========================================
- Coverage   95.70%   94.32%   -1.39%     
==========================================
  Files         101      103       +2     
  Lines       26284    26669     +385     
==========================================
  Hits        25155    25155              
- Misses       1129     1514     +385     
Files with missing lines Coverage Δ
desc/experimental/__init__.py 0.00% <0.00%> (ø)
desc/experimental/_terpsichore.py 0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
funtionality New feature or request to do things the code can't do now. interface New feature or request to make the code more usable or compatibility with another code objectives Adding or improving objective functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

directory for external code
6 participants