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

signed curvature #1085

Merged
merged 28 commits into from
Jul 16, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
72195f8
initial commit
daniel-dudt Jun 27, 2024
572766a
Merge branch 'yge/rpz2xyz' into dd/curvature
daniel-dudt Jun 27, 2024
10ff1d9
Merge branch 'yge/rpz2xyz' into dd/curvature
daniel-dudt Jun 27, 2024
fc4bbf6
fix formulas
daniel-dudt Jun 27, 2024
bc3f2b9
Merge branch 'yge/rpz2xyz' into dd/curvature
ddudt Jun 28, 2024
3a768d3
Merge branch 'yge/rpz2xyz' into dd/curvature
dpanici Jul 3, 2024
50bea28
Merge branch 'yge/rpz2xyz' into dd/curvature
ddudt Jul 10, 2024
d6d4b4b
fix bug and test for compute center
daniel-dudt Jul 10, 2024
64f48fb
Merge branch 'master' into dd/curvature
ddudt Jul 11, 2024
9c4b3b1
Merge branch 'master' into dd/curvature
ddudt Jul 11, 2024
9591eb2
debugging curvature sign
daniel-dudt Jul 12, 2024
fe0d15b
Merge branch 'master' into dd/curvature
ddudt Jul 12, 2024
bc38b60
fix bug in center computations
daniel-dudt Jul 12, 2024
9a6e278
update test even though it doesn't really matter
daniel-dudt Jul 12, 2024
6715aef
make the code more readable
daniel-dudt Jul 12, 2024
e8b95aa
Merge branch 'master' into dd/curvature
ddudt Jul 12, 2024
5fa3338
fixing tests
daniel-dudt Jul 12, 2024
abc4a42
test frenet in xyz basis
daniel-dudt Jul 12, 2024
2c4ad5c
consistent naming convention of coil/coils
daniel-dudt Jul 15, 2024
ca62ef1
fix bug in coil objective build order
daniel-dudt Jul 15, 2024
f293c62
generalize LinearObjectiveFromUser to work with OptimizableCollection
daniel-dudt Jul 15, 2024
a40ea3b
remove unused code
daniel-dudt Jul 15, 2024
8801c4d
fix coil kwargs in tests
daniel-dudt Jul 15, 2024
6fe0663
Merge branch 'master' into dd/curvature
ddudt Jul 16, 2024
da23cc4
making requested changes
daniel-dudt Jul 16, 2024
12465c1
clarify CoilCurvature sign convention in docs
daniel-dudt Jul 16, 2024
5ed70e0
update master compute data, again
daniel-dudt Jul 16, 2024
fc5d105
fix test
dpanici Jul 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 55 additions & 10 deletions desc/compute/_curve.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from interpax import interp1d

from desc.backend import jnp
from desc.backend import jnp, sign

from .data_index import register_compute_fun
from .geom_utils import rotation_matrix, rpz2xyz, rpz2xyz_vec, xyz2rpz, xyz2rpz_vec
Expand Down Expand Up @@ -146,6 +146,50 @@ def _Z_Curve(params, transforms, profiles, data, **kwargs):
return data


@register_compute_fun(
name="center",
label="\\langle\\mathbf{x}\\rangle",
units="m",
units_long="meters",
description="Centroid of the curve",
dim=3,
params=[],
transforms={},
profiles=[],
coordinates="s",
data=["x"],
parameterization="desc.geometry.core.Curve",
)
def _center_Curve(params, transforms, profiles, data, **kwargs):
data["center"] = jnp.mean(data["x"], axis=0) * jnp.ones_like(data["x"])
ddudt marked this conversation as resolved.
Show resolved Hide resolved
return data


@register_compute_fun(
name="center",
label="\\langle\\mathbf{x}\\rangle",
units="m",
units_long="meters",
description="Centroid of the curve",
dim=3,
params=["center", "shift"],
transforms={},
profiles=[],
coordinates="s",
data=["x"],
parameterization="desc.geometry.curve.FourierPlanarCurve",
basis_in="{'rpz', 'xyz'}: Basis for input params vectors, Default 'xyz'",
)
def _center_FourierPlanarCurve(params, transforms, profiles, data, **kwargs):
# convert to rpz
if kwargs.get("basis_in", "xyz").lower() == "rpz":
center = params["center"]
else:
center = xyz2rpz(params["center"])
data["center"] = (center + xyz2rpz(params["shift"])) * jnp.ones_like(data["x"])
return data


@register_compute_fun(
name="x",
label="\\mathbf{x}",
Expand Down Expand Up @@ -790,13 +834,12 @@ def _frenet_tangent(params, transforms, profiles, data, **kwargs):
transforms={},
profiles=[],
coordinates="s",
data=["x_ss"],
data=["x_s", "x_ss", "frenet_tangent"],
ddudt marked this conversation as resolved.
Show resolved Hide resolved
parameterization="desc.geometry.core.Curve",
)
def _frenet_normal(params, transforms, profiles, data, **kwargs):
data["frenet_normal"] = (
data["x_ss"] / jnp.linalg.norm(data["x_ss"], axis=-1)[:, None]
)
normal = cross(data["x_s"], cross(data["x_ss"], data["x_s"]))
data["frenet_normal"] = normal / jnp.linalg.norm(normal, axis=-1)[:, None]
return data


Expand Down Expand Up @@ -826,20 +869,22 @@ def _frenet_binormal(params, transforms, profiles, data, **kwargs):
label="\\kappa",
units="m^{-1}",
units_long="Inverse meters",
description="Scalar curvature of the curve",
description="Scalar curvature of the curve, positive/negative = concave/convex",
ddudt marked this conversation as resolved.
Show resolved Hide resolved
dim=1,
params=[],
transforms={},
profiles=[],
coordinates="s",
data=["x_s", "x_ss"],
data=["center", "x", "x_s", "x_ss", "frenet_normal"],
parameterization="desc.geometry.core.Curve",
)
def _curvature(params, transforms, profiles, data, **kwargs):
# magnitude of curvature
dxn = jnp.linalg.norm(data["x_s"], axis=-1)[:, jnp.newaxis]
data["curvature"] = jnp.linalg.norm(
cross(data["x_s"], data["x_ss"]) / dxn**3, axis=-1
)
curvature = jnp.linalg.norm(cross(data["x_s"], data["x_ss"]) / dxn**3, axis=-1)
# sign of curvature (positive = "concave", negative = "convex")
ddudt marked this conversation as resolved.
Show resolved Hide resolved
r = data["x"] - data["center"]
data["curvature"] = curvature * sign(dot(r, data["frenet_normal"]))
return data


Expand Down
12 changes: 6 additions & 6 deletions desc/objectives/_coils.py
Original file line number Diff line number Diff line change
Expand Up @@ -332,8 +332,9 @@ def compute(self, params, constants=None):
class CoilCurvature(_CoilObjective):
"""Coil curvature.

Targets the local curvature value per grid node for each coil. A smaller curvature
value indicates straighter coils. All curvature values are positive.
Targets the local curvature at each grid node for each coil. Positive curvature
corresponds to "concave" curves, while negative curvature corresponds to "convex"
curves. Curvature values closer to 0 indicate straighter coils.

Parameters
----------
Expand Down Expand Up @@ -392,7 +393,7 @@ def __init__(
name="coil curvature",
):
if target is None and bounds is None:
bounds = (0, 1)
bounds = (-1, 0)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


super().__init__(
coil,
Expand Down Expand Up @@ -450,9 +451,8 @@ def compute(self, params, constants=None):
class CoilTorsion(_CoilObjective):
"""Coil torsion.

Targets the local torsion value per grid node for each coil. Indicative
of how much the coil goes out of the poloidal plane. e.g. a torsion
value of 0 means the coil is completely planar.
Targets the local torsion value at each grid node for each coil. Indicative of how
non-planar the coil is (a torsion value of 0 means the coil is perfectly planar).

Parameters
----------
Expand Down
2 changes: 1 addition & 1 deletion tests/test_objective_funs.py
Original file line number Diff line number Diff line change
Expand Up @@ -825,7 +825,7 @@ def test(coil, grid=None):
obj = CoilCurvature(coil, grid=grid)
obj.build()
f = obj.compute(params=coil.params_dict)
np.testing.assert_allclose(f, 1 / 2, rtol=1e-8)
np.testing.assert_allclose(f, -1 / 2, rtol=1e-8)
assert len(f) == obj.dim_f

coil = FourierPlanarCoil(r_n=2)
Expand Down