Jax curve pr #528

akaptano · 2025-05-28T13:21:15Z

Adds in functionality from force_and_torque_overhaul PR #509, including:

jax curves, including JaxCurvePlanarFourier
Adds tests of all the jax functionality, including improving the coverage of JaxCurveXYZFourier
Added make_names and other improvements to JaxCurveXYZFourier, such as faster (vectorized) gamma_pure calculation
Adds in better docstrings and downsample parameter for some of the expensive curve objectives.

…meter.

codecov · 2025-05-28T16:28:01Z

Codecov Report

Attention: Patch coverage is 99.14894% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.64%. Comparing base (ed66b1d) to head (427bfbf).
Report is 14 commits behind head on master.

Files with missing lines	Patch %	Lines
src/simsopt/geo/curve.py	98.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #528      +/-   ##
==========================================
+ Coverage   92.55%   92.64%   +0.09%     
==========================================
  Files          83       83              
  Lines       16027    16190     +163     
==========================================
+ Hits        14834    15000     +166     
+ Misses       1193     1190       -3

Flag	Coverage Δ
unittests	`92.64% <99.14%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

src/simsopt/geo/curveplanarfourier.py

src/simsopt/geo/curvexyzfourier.py

tests/geo/test_curve_objectives.py

src/simsopt/geo/curve.py

mishapadidar

What is the create_planar_curves_between_two_toroidal_surfaces for?

src/simsopt/geo/curve.py

mishapadidar

just small comments.

tests/geo/test_curve.py

mishapadidar · 2025-06-12T15:29:22Z

tests/geo/test_curve.py

+        TEST_DIR = (Path(__file__).parent / ".." / "test_files").resolve()
+        filename = TEST_DIR / 'input.LandremanPaul2021_QA'
+        nphi, ntheta = 8, 8
+        with ScratchDir("."):


should we be using a different directory for scratchdir?

Need to fix this elsewhere too then

tests/geo/test_curve_objectives.py

src/simsopt/geo/curvexyzfourier.py

…a suggestions.

akaptano · 2025-06-25T17:44:39Z

@landreman @mishapadidar Last call -- please approve if you don't see anything else. Thanks!

akaptano · 2025-07-04T14:29:52Z

Reminder to please approve!

landreman · 2025-07-06T15:59:53Z

src/simsopt/geo/curve.py

        """
        return self.curve.num_dofs()
+
+    def centroid(self):


The base Curve class already has the same centroid function, and JaxCurve inherits from the base Curve class. So I would guess this function in JaxCurve is superfluous - can it be removed?

Looking at this again, I do not see a JaxCurve version of the function. The lines you highlighted above seem to be for a RotatedCurve object. For this, I think the RotatedCurve should not inherit, since it should call centroid_pure with its own gamma and gammadash, which are wrappers over the inherited curve's gamma and gammadash. Let me know if that is not right though!

Im a bit confused here. It seems like the right thing to do is for the Curve class to have a centroid function, and the JaxCurve class to have centroid_pure. Then the RotataedCurve can just inherit from Curve, right?

What is wrong with having centroid_pure for the Curve, and then both JaxCurve and RotatedCurve inheriting that function?

I agree with Alan - the present arrangement with centroid() in the base Curve class looks good.

src/simsopt/geo/curve.py

mishapadidar

code mostly looks good. My main concern is with the downsample parameter in the CurveCurveDistance class. If there is another, cleaner, approach to reducing the computational cost of the CurveCurveDistance, it is welcome.

tests/geo/test_curve.py

tests/field/test_magneticfields.py

src/simsopt/geo/curvexyzfourier.py

mishapadidar · 2025-07-10T15:18:38Z

src/simsopt/geo/curve.py

        """
        return self.curve.num_dofs()
+
+    def centroid(self):


Im a bit confused here. It seems like the right thing to do is for the Curve class to have a centroid function, and the JaxCurve class to have centroid_pure. Then the RotataedCurve can just inherit from Curve, right?

landreman · 2025-07-10T17:59:54Z

Looks good to me now. But ugh the CI is failing now for some reason that's probably unrelated to this PR. Let's see if we can fix the CI in the ml/fix_CI_20250710 branch (derived from master)

akaptano · 2025-07-10T20:22:22Z

@landreman @mbkumar Suddenly the C++ build seems to be failing here, on code that I have not touched. Any idea why? Oops sorry just saw your comment Matt, thanks.

mbkumar · 2025-07-10T20:26:48Z

Pybind11 released a new version today with an ABI change. Reverting back to older pybind11 should fix the issue

…

On Thu, Jul 10, 2025, 4:22 PM Alan Kaptanoglu ***@***.***> wrote: *akaptano* left a comment (hiddenSymmetries/simsopt#528) <#528 (comment)> @landreman <https://github.com/landreman> @mbkumar <https://github.com/mbkumar> Suddenly the C++ build seems to be failing here, on code that I have not touched. Any idea why? — Reply to this email directly, view it on GitHub <#528 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA62VEF34UUWZUNE7JZXJUD3H3DRLAVCNFSM6AAAAAB6C2FJUGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTANJYHE2DGOBYHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

akaptano · 2025-07-11T19:04:18Z

Thanks. Just fixed that and this PR should be ready for approval.

akaptano added 2 commits May 28, 2025 07:15

Added the changes with jaxcurves and additional tests.

82a462e

Added new curveobjectives with updated docstrings and downsample para…

a0250ca

…meter.

akaptano requested review from landreman and mishapadidar May 28, 2025 13:27

akaptano self-assigned this May 28, 2025

Accidentally deleted wf test file.

72e0872

landreman reviewed May 28, 2025

View reviewed changes

src/simsopt/geo/curveplanarfourier.py Outdated Show resolved Hide resolved

akaptano added 2 commits May 29, 2025 08:33

Fix merge fixes in the curveplanarfourier docstring.

bb187b1

Merge branch 'master' into jax_curve_PR

de30187

landreman reviewed May 31, 2025

View reviewed changes

mishapadidar reviewed Jun 2, 2025

View reviewed changes

akaptano added 2 commits June 12, 2025 09:05

Fixed things from matt and misha suggestions.

870aba8

Merge branch 'master' into jax_curve_PR

3fb044b

mishapadidar reviewed Jun 12, 2025

View reviewed changes

ADded better unit test error messages for curves, along lines of Mish…

dcf805c

…a suggestions.

akaptano requested a review from mbkumar July 7, 2025 13:03

landreman self-requested a review July 7, 2025 14:04

landreman reviewed Jul 7, 2025

View reviewed changes

Made docstring change suggested by Matt in the PR.

c3a053f

mishapadidar reviewed Jul 10, 2025

View reviewed changes

Made last docstring and other changes in the PR.

1a620a9

akaptano added 2 commits July 11, 2025 11:46

Merge branch 'master' into jax_curve_PR

840493c

Fixed silly syntax error.

427bfbf

landreman self-requested a review July 11, 2025 20:26

landreman approved these changes Jul 11, 2025

View reviewed changes

akaptano merged commit 6391891 into master Jul 11, 2025
48 of 50 checks passed

Uh oh!

Jax curve pr #528

Jax curve pr #528

Uh oh!

Conversation

akaptano commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mishapadidar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mishapadidar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mishapadidar Jun 12, 2025

Choose a reason for hiding this comment

Uh oh!

akaptano Jun 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

akaptano commented Jun 25, 2025

Uh oh!

akaptano commented Jul 4, 2025

Uh oh!

landreman Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akaptano Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

mishapadidar Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

akaptano Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

landreman Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mishapadidar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

akaptano commented May 28, 2025 •

edited

Loading

codecov bot commented May 28, 2025 •

edited

Loading

landreman Jul 6, 2025 •

edited

Loading

akaptano commented Jul 10, 2025 •

edited

Loading