Skip to content

Adding QUADCOIL integration to DESC as an objective - #2110

Open
lankef wants to merge 72 commits into
masterfrom
ffu/quadcoil-qss-pre-commit
Open

Adding QUADCOIL integration to DESC as an objective#2110
lankef wants to merge 72 commits into
masterfrom
ffu/quadcoil-qss-pre-commit

Conversation

@lankef

@lankef lankef commented Mar 3, 2026

Copy link
Copy Markdown
Collaborator

This pull request adds two files under /objective, _quadcoil.py and _quadcoil_utils.py. These allows one to call QUADCOIL using a DESC equilibrium and filament coil set as input, and perform quasi-single-stage optimization on the equilibrium and/or the coilset. There are some additions in coils and curves for conversion from simsopt objects. (since quadcoil is written in the simsopt convention). Winding surface can be either fixed or auto-generated, but optimizing the geometry of the winding surface is not yet implemented. The unit test compares quadcoil to desc's built-in REGCOIL.

lankef and others added 30 commits April 4, 2025 12:23
… optimizable_io.py and objective_funs.py to avoid error for static, empty tuple attributes and _objectives with self.coordinates == \'\'
…nversion, but it's not working because quadcoilproxy does not directly store the input dictionary, and desc static dict cannot contain strings (?)
…: Colorbar layout of new layout engine not compatible with old engine, and a colorbar has been created. Engine not changed.

@YigitElma YigitElma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks fore adding this!
I will review rest of the code later, but it would be nice if you can resolve the problem with CI tests first. It looks like CI and pip in general cannot install quadcoil, I couldn't install it locally either.

Comment thread desc/objectives/objective_funs.py Outdated
"_use_jit",
]
_static_attrs = [
"_static_attrs", # A bug as of Oct 10 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

_static_attrs is already included couple lines below. You probably had to add it before we added in master.

Comment thread desc/objectives/objective_funs.py Outdated
Comment thread desc/plotting.py Outdated
Comment thread desc/io/optimizable_io.py
def _unmake_hashable(x):
# turn tuple of ints and shape to ndarray
if isinstance(x, tuple) and x[0] == "ndarray":
if isinstance(x, tuple) and len(x) and x[0] == "ndarray":

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think it hurts to have it, but why did you need this change?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The default values for some QUADCOIL arguments are empty list/tuples (I had to do it because the adjoint derivative is wrapped in a vjp rule before handed to DESC and this way some flows are simpler). Without this check it breaks DESC Optimizable.

Comment thread devtools/dev-requirements.txt
Comment thread desc/geometry/curve.py Outdated
Comment thread desc/objectives/_quadcoil.py Outdated
Comment thread desc/objectives/_quadcoil.py Outdated
metric_name,
value_only,
verbose,
plasma_M_theta : int, optional, default=eq.M_grid

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We usually give defaults at the end of the description

Comment thread desc/objectives/_quadcoil.py Outdated
bs_chunk_size : int, optional, default=None
Size to split Biot-Savart computation into chunks of evaluation points.

Attributes

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These don't seem necessary

Comment thread desc/objectives/_quadcoil.py Outdated
# we set them here. This is necessary because we are calling quadcoil
# through quadcoil.io.quadcoil_for_diff, which cannot see their default
# values in quadcoil.quadcoil.
if "net_toroidal_current_amperes" in quadcoil_kwargs.keys():

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You can replace all if/else statements by something = dict.pop("something", default)

Comment thread desc/objectives/_quadcoil.py
Comment thread desc/objectives/_quadcoil.py Outdated
self._quadcoil_for_diff = jit(_quadcoil_for_diff)
self._quadcoil_values = jit(_quadcoil_values)
# ----- Setting and registering keyword arguments -----
self._static_attrs = _Objective._static_attrs + [

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You can move this to line 165, right after docs. We usually define it there

Comment thread desc/objectives/_quadcoil.py Outdated
Comment thread desc/objectives/_quadcoil_utils.py Outdated
Comment thread desc/objectives/_quadcoil.py Outdated
lankef and others added 5 commits May 2, 2026 13:22
Co-authored-by: Yigit Gunsur Elmacioglu <102380275+YigitElma@users.noreply.github.com>
…dict popping with defaults using pop(default=**) to improve readability
@YigitElma

Copy link
Copy Markdown
Collaborator

@lankef https://github.com/PlasmaControl/DESC/actions/runs/25258105132/job/74060745121?pr=2110 this error happens if you commit a notebook from cluster or after running notebook in Jupyter Lab session (tbh it is very annoying and we have a long standing issue about this #1215, but it is actually not our problem but nbmake or jupyterlab's). You can check this related comment #1498 (comment) for fix. Basically change kernel name of the notebook metadata or just open/save (without running it) on VS code extension.

lankef and others added 17 commits August 12, 2026 14:46
Merging main changes to branch

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Yigit Gunsur Elmacioglu <102380275+YigitElma@users.noreply.github.com>
Co-authored-by: Kaya Unalmis <kayaunalmis@proton.me>
Co-authored-by: Matthew Feickert <matthew.feickert@cern.ch>
Co-authored-by: Dario Panici <37969854+dpanici@users.noreply.github.com>
Co-authored-by: YigitElma <yigitelmacioglu@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Dario Panici <dpanici@princeton.edu>
Co-authored-by: Rory Conlin <mail@fouriest.net>
Co-authored-by: Daniel Dudt <33005725+ddudt@users.noreply.github.com>
Co-authored-by: daniel-dudt <daniel.dudt@princetonstellarators.energy>
Co-authored-by: Jonathan Brodrick <jonathan.brodrick@simulation.science>
Co-authored-by: Wenyin Wei <wenyin.wei.ww@gmail.com>
Co-authored-by: Jaydeep Singh <jaydeepsingh15@gmail.com>
Co-authored-by: Jonathan Brodrick <jonathanbrodrick@gmail.com>
Co-authored-by: Daniel Dudt <135651230+daniel-dudt@users.noreply.github.com>
Co-authored-by: issraali <issra.ali@mail.utoronto.ca>
Co-authored-by: T.Thun <timothun@web.de>
Co-authored-by: Yigit Gunsur Elmacioglu <102380275+YigitElma@users.noreply.github.com>
2.  Changing import to use desc.backend
3. Fixing notebook issue
@codecov

codecov Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 64.97696% with 152 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.94%. Comparing base (ef795c5) to head (8aa5ac6).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
desc/objectives/_quadcoil.py 69.26% 71 Missing ⚠️
desc/objectives/_quadcoil_utils.py 67.05% 57 Missing ⚠️
desc/geometry/curve.py 8.69% 21 Missing ⚠️
desc/coils.py 40.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2110      +/-   ##
==========================================
+ Coverage   84.50%   93.94%   +9.44%     
==========================================
  Files         101      103       +2     
  Lines       28953    29386     +433     
==========================================
+ Hits        24466    27607    +3141     
+ Misses       4487     1779    -2708     
Files with missing lines Coverage Δ
desc/io/optimizable_io.py 87.05% <100.00%> (+0.58%) ⬆️
desc/objectives/__init__.py 100.00% <100.00%> (ø)
desc/coils.py 97.09% <40.00%> (+6.99%) ⬆️
desc/geometry/curve.py 93.56% <8.69%> (+13.97%) ⬆️
desc/objectives/_quadcoil_utils.py 67.05% <67.05%> (ø)
desc/objectives/_quadcoil.py 69.26% <69.26%> (ø)

... and 67 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@YigitElma

Copy link
Copy Markdown
Collaborator

I made some small changes to fix the failing test and fixed a couple of bugs (for example, classmethods expect cls as the first argument). In the notebook, I added a helper function for plotting, I think you can use it instead of duplicated code (I just added the helper, but didn't change the code to use it yet). Also, it looks like the notebook is currently trying to use 127GB of memory which is not possible to run on CI. I added jac_chunk_size=1 to QuadcoilProxy objective, but we may need to reduce the resolution or something else to keep it short. For CI purposes, we want notebooks to run in around 10 minutes.

One additional question for other devs: do we want this in the desc/objectives module or desc/external module? @dpanici @f0uriest @ddudt

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.

5 participants