signed curvature #1085

ddudt · 2024-06-27T21:34:59Z

Resolves #1039

Also fixes a bug in FourierRZCurve.compute(["x_s", "x_ss", "x_sss"]).

Need to add tests for the following:

new compute quantity "center"
FourierPlanarCoil with both "xyz" and "rpz" basis
update CoilCurvature objective tests

desc/compute/_curve.py

github-actions · 2024-07-11T21:13:23Z

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     +4.63 +/- 7.31     | +2.39e-02 +/- 3.78e-02 |  5.41e-01 +/- 3.6e-02  |  5.17e-01 +/- 1.3e-02  |
 test_build_transform_fft_midres         |     -0.85 +/- 5.61     | -5.26e-03 +/- 3.49e-02 |  6.16e-01 +/- 1.7e-02  |  6.21e-01 +/- 3.0e-02  |
 test_build_transform_fft_highres        |     -3.10 +/- 2.78     | -3.24e-02 +/- 2.90e-02 |  1.01e+00 +/- 2.1e-02  |  1.04e+00 +/- 2.0e-02  |
 test_equilibrium_init_lowres            |     +6.80 +/- 4.68     | +2.59e-01 +/- 1.78e-01 |  4.07e+00 +/- 9.3e-02  |  3.81e+00 +/- 1.5e-01  |
 test_equilibrium_init_medres            |     +0.96 +/- 3.47     | +4.06e-02 +/- 1.47e-01 |  4.28e+00 +/- 1.0e-01  |  4.24e+00 +/- 1.0e-01  |
 test_equilibrium_init_highres           |     -0.00 +/- 1.71     | -7.19e-05 +/- 9.61e-02 |  5.63e+00 +/- 4.8e-02  |  5.63e+00 +/- 8.3e-02  |
 test_objective_compile_dshape_current   |     +0.90 +/- 3.47     | +3.47e-02 +/- 1.34e-01 |  3.89e+00 +/- 2.7e-02  |  3.85e+00 +/- 1.3e-01  |
 test_objective_compile_atf              |     -0.99 +/- 1.79     | -8.38e-02 +/- 1.51e-01 |  8.38e+00 +/- 1.2e-01  |  8.47e+00 +/- 9.1e-02  |
 test_objective_compute_dshape_current   |     -1.55 +/- 3.75     | -1.97e-05 +/- 4.76e-05 |  1.25e-03 +/- 3.9e-05  |  1.27e-03 +/- 2.8e-05  |
 test_objective_compute_atf              |    -11.73 +/- 6.37     | -5.72e-04 +/- 3.11e-04 |  4.31e-03 +/- 1.5e-04  |  4.88e-03 +/- 2.7e-04  |
 test_objective_jac_dshape_current       |     +1.31 +/- 10.87    | +4.92e-04 +/- 4.10e-03 |  3.82e-02 +/- 3.7e-03  |  3.77e-02 +/- 1.8e-03  |
 test_objective_jac_atf                  |     +0.65 +/- 4.69     | +1.23e-02 +/- 8.91e-02 |  1.91e+00 +/- 4.5e-02  |  1.90e+00 +/- 7.7e-02  |
 test_perturb_1                          |     +0.96 +/- 3.30     | +1.31e-01 +/- 4.51e-01 |  1.38e+01 +/- 4.0e-01  |  1.37e+01 +/- 2.1e-01  |
 test_perturb_2                          |     +1.18 +/- 1.55     | +2.17e-01 +/- 2.84e-01 |  1.86e+01 +/- 2.0e-01  |  1.84e+01 +/- 2.0e-01  |
 test_proximal_jac_atf                   |     +1.37 +/- 1.15     | +1.01e-01 +/- 8.55e-02 |  7.51e+00 +/- 6.5e-02  |  7.40e+00 +/- 5.6e-02  |
 test_proximal_freeb_compute             |     -1.30 +/- 0.85     | -2.33e-03 +/- 1.53e-03 |  1.77e-01 +/- 9.6e-04  |  1.79e-01 +/- 1.2e-03  |
 test_proximal_freeb_jac                 |     +0.63 +/- 1.55     | +4.70e-02 +/- 1.15e-01 |  7.44e+00 +/- 1.1e-01  |  7.40e+00 +/- 4.0e-02  |
 test_solve_fixed_iter                   |     -5.34 +/- 7.34     | -8.48e-01 +/- 1.17e+00 |  1.50e+01 +/- 6.4e-01  |  1.59e+01 +/- 9.8e-01  |

f0uriest

few typos, main comment is about the sign convention and possible confusion with people used to unsigned curvature.

Also, suppose we flipped our parameterization s -> -s, then the frenet frame would flip, and wouldn't that make the curvature change sign? even though the curve is the same?

desc/compute/_curve.py

f0uriest · 2024-07-12T19:23:17Z

desc/objectives/_coils.py

@@ -395,7 +396,7 @@ def __init__(
        name="coil curvature",
    ):
        if target is None and bounds is None:
-            bounds = (0, 1)
+            bounds = (-1, 0)


might want to throw a warning if the user passes in bounds like (0, max) since that wouldn't be possible i think (if a circle has curvature -1)? For people used to unsigned curvature this might be confusing. Or maybe we just flip our sign convention so that a circle has positive curvature? though that slighlty conflicts with our convention for surface curvature

could also have a flag for whether to use signed or unsigned curvature?

I wish our sign convention was flipped (positive = convex), but I went with the sign convention we already use for the surface curvature. I would be fine with changing them both.

A closed curve has to be convex at some points, but in theory you could target a curve to be locally concave in certain regions. And I think there is a conceivable use case for targeting positive curvature everywhere, even though it is not possible, to try and force the coils to be straight (low curvature everywhere).

Instead of a flag for signed/unsigned, I think the better solution would be to add a loss_function="abs" option. But that can be a separate PR.

I agree the sign convention is annoying, I based it on several textbooks which define positive curvature of a surface as curving towards the outward normal vector to the surface.

I don't think we necessarily have to keep the same convention for curves, since signed curvature for non-planar curves isn't even necessarily well defined, and to me it makes more sense to define it such that a circle has positive curvature.

At the very least, I think we should include a note in the docstring for the curvature objective reminding users of the sign convention.

ddudt · 2024-07-12T21:31:57Z

few typos, main comment is about the sign convention and possible confusion with people used to unsigned curvature.

Also, suppose we flipped our parameterization s -> -s, then the frenet frame would flip, and wouldn't that make the curvature change sign? even though the curve is the same?

If we flip the parameterization, the tangent vector will flip but the normal vector will still point inwards towards the center of local curvature. The normal vector is what gets used to determine the sign of the curvature, so I think this should be fine.

desc/objectives/_coils.py

dpanici

address changes and it will be good to go

desc/compute/_curve.py

desc/objectives/_coils.py

dpanici · 2024-07-16T18:26:17Z

desc/objectives/_generic.py

        J2 = jax.jacrev(self._fun)(params)
-        for key in J1.keys():
-            assert np.all(J1[key] == J2[key]), "Function must be linear!"
+        for j1, j2 in zip(tree_leaves(J1), tree_leaves(J2)):


why was this change needed?

This is technically unrelated to the signed curvature, but I threw it in this PR (sorry). This change is necessary to allow LinearObjectiveFromUser to work with an OptimizableCollection like a CoilSet.

is this tested anywhere in this PR?

The new functionality is not tested in this PR. But there are existing tests for LinearObjectiveFromUser that still pass.

see #1130 for a test of this new functionality

tests/test_curves.py

dpanici · 2024-07-16T18:32:43Z

And fix the docs

kianorr · 2024-07-16T21:16:33Z

desc/coils.py

@@ -1665,7 +1670,7 @@ def __init__(self, *coils, name="", check_intersection=True):
    @property
    def num_coils(self):
        """int: Number of coils."""
-        return sum([c.num_coils if hasattr(c, "num_coils") else 1 for c in self])
+        return sum([c.num_coils for c in self])


Does this work for a nested coilset?

I think it should, because num_coils will just get called recursively until the self is a single coilset

dpanici

If there was a change for linear objective from user to have it work with optimizable collections, I'd like to see it tested if possible. If not in this PR then make an issue to test it in the future

ddudt · 2024-07-16T22:11:17Z

If there was a change for linear objective from user to have it work with optimizable collections, I'd like to see it tested if possible. If not in this PR then make an issue to test it in the future

I just added a test for it in #1130, which will now fail until this PR is merged.

daniel-dudt added 4 commits June 27, 2024 12:38

initial commit

72195f8

Merge branch 'yge/rpz2xyz' into dd/curvature

572766a

Merge branch 'yge/rpz2xyz' into dd/curvature

10ff1d9

fix formulas

fc4bbf6

ddudt added the coil stuff relating to coils and coil optimization label Jun 27, 2024

ddudt self-assigned this Jun 27, 2024

f0uriest reviewed Jun 28, 2024

View reviewed changes

desc/compute/_curve.py Outdated Show resolved Hide resolved

Merge branch 'yge/rpz2xyz' into dd/curvature

bc3f2b9

unalmis added the bug fix Something was fixed label Jul 1, 2024

dpanici and others added 3 commits July 3, 2024 15:39

Merge branch 'yge/rpz2xyz' into dd/curvature

3a768d3

Merge branch 'yge/rpz2xyz' into dd/curvature

50bea28

fix bug and test for compute center

d6d4b4b

ddudt commented Jul 10, 2024

View reviewed changes

desc/compute/_curve.py Show resolved Hide resolved

Base automatically changed from yge/rpz2xyz to master July 11, 2024 17:39

ddudt added 2 commits July 11, 2024 13:54

Merge branch 'master' into dd/curvature

64f48fb

Merge branch 'master' into dd/curvature

9c4b3b1

daniel-dudt and others added 4 commits July 11, 2024 20:24

debugging curvature sign

9591eb2

Merge branch 'master' into dd/curvature

fe0d15b

fix bug in center computations

bc38b60

update test even though it doesn't really matter

9a6e278

ddudt marked this pull request as ready for review July 12, 2024 18:12

ddudt requested review from f0uriest, dpanici and kianorr July 12, 2024 18:12

f0uriest requested changes Jul 12, 2024

View reviewed changes

make the code more readable

6715aef

ddudt and others added 2 commits July 12, 2024 15:32

Merge branch 'master' into dd/curvature

e8b95aa

fixing tests

5fa3338

daniel-dudt and others added 4 commits July 15, 2024 15:02

generalize LinearObjectiveFromUser to work with OptimizableCollection

f293c62

remove unused code

a40ea3b

fix coil kwargs in tests

8801c4d

Merge branch 'master' into dd/curvature

6fe0663

ddudt mentioned this pull request Jul 16, 2024

Add Stage-Two Coil Optimization Tutorial #1114

Merged

ddudt commented Jul 16, 2024

View reviewed changes

desc/objectives/_coils.py Show resolved Hide resolved

ddudt commented Jul 16, 2024

View reviewed changes

desc/objectives/_coils.py Show resolved Hide resolved

dpanici requested changes Jul 16, 2024

View reviewed changes

making requested changes

da23cc4

ddudt requested a review from dpanici July 16, 2024 19:15

daniel-dudt added 2 commits July 16, 2024 13:54

clarify CoilCurvature sign convention in docs

12465c1

update master compute data, again

5ed70e0

kianorr reviewed Jul 16, 2024

View reviewed changes

fix test

fc5d105

dpanici requested changes Jul 16, 2024

View reviewed changes

dpanici mentioned this pull request Jul 16, 2024

Test new functionality for LinearObjectiveFromUser #1131

Closed

dpanici approved these changes Jul 16, 2024

View reviewed changes

ddudt requested review from dpanici and kianorr and removed request for dpanici July 16, 2024 22:11

dpanici requested review from kianorr and f0uriest and removed request for f0uriest and kianorr July 16, 2024 22:45

kianorr approved these changes Jul 16, 2024

View reviewed changes

f0uriest approved these changes Jul 16, 2024

View reviewed changes

ddudt merged commit a02cf04 into master Jul 16, 2024
18 checks passed

ddudt deleted the dd/curvature branch July 16, 2024 23:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

signed curvature #1085

signed curvature #1085

ddudt commented Jun 27, 2024 •

edited

Loading

github-actions bot commented Jul 11, 2024 •

edited

Loading

f0uriest left a comment

f0uriest Jul 12, 2024

f0uriest Jul 12, 2024

ddudt Jul 12, 2024

f0uriest Jul 15, 2024

ddudt commented Jul 12, 2024

dpanici left a comment

dpanici Jul 16, 2024

ddudt Jul 16, 2024

dpanici Jul 16, 2024

ddudt Jul 16, 2024 •

edited

Loading

ddudt Jul 16, 2024

dpanici commented Jul 16, 2024

kianorr Jul 16, 2024

dpanici Jul 16, 2024

dpanici left a comment

ddudt commented Jul 16, 2024

signed curvature #1085

signed curvature #1085

Conversation

ddudt commented Jun 27, 2024 • edited Loading

github-actions bot commented Jul 11, 2024 • edited Loading

f0uriest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddudt commented Jul 12, 2024

dpanici left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddudt Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpanici commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpanici left a comment

Choose a reason for hiding this comment

ddudt commented Jul 16, 2024

ddudt commented Jun 27, 2024 •

edited

Loading

github-actions bot commented Jul 11, 2024 •

edited

Loading

ddudt Jul 16, 2024 •

edited

Loading