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

C0 coils with splines #970

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

C0 coils with splines #970

wants to merge 103 commits into from

Conversation

kianorr
Copy link
Collaborator

@kianorr kianorr commented Mar 29, 2024

Add discontinuous knots for C0 capabilities.

kianorr and others added 4 commits March 7, 2024 11:58
- also added to compute function for each interval
- didn't add for all compute functions yet
desc/geometry/curve.py Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Mar 30, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     +3.41 +/- 6.95     | +1.77e-02 +/- 3.61e-02 |  5.38e-01 +/- 3.4e-02  |  5.20e-01 +/- 1.2e-02  |
 test_build_transform_fft_midres         |     +3.77 +/- 6.87     | +2.28e-02 +/- 4.15e-02 |  6.27e-01 +/- 3.4e-02  |  6.04e-01 +/- 2.4e-02  |
 test_build_transform_fft_highres        |     +1.32 +/- 5.00     | +1.33e-02 +/- 5.03e-02 |  1.02e+00 +/- 3.9e-02  |  1.01e+00 +/- 3.2e-02  |
 test_equilibrium_init_lowres            |     +5.62 +/- 5.84     | +2.17e-01 +/- 2.26e-01 |  4.09e+00 +/- 2.0e-01  |  3.87e+00 +/- 1.1e-01  |
 test_equilibrium_init_medres            |     +4.43 +/- 6.27     | +1.87e-01 +/- 2.65e-01 |  4.41e+00 +/- 1.8e-01  |  4.22e+00 +/- 2.0e-01  |
 test_equilibrium_init_highres           |     +2.12 +/- 5.26     | +1.18e-01 +/- 2.92e-01 |  5.67e+00 +/- 2.0e-01  |  5.55e+00 +/- 2.1e-01  |
 test_objective_compile_dshape_current   |     +3.76 +/- 2.82     | +1.45e-01 +/- 1.08e-01 |  3.99e+00 +/- 7.2e-02  |  3.85e+00 +/- 8.1e-02  |
 test_objective_compile_atf              |     -1.70 +/- 2.63     | -1.38e-01 +/- 2.13e-01 |  7.96e+00 +/- 1.1e-01  |  8.10e+00 +/- 1.8e-01  |
 test_objective_compute_dshape_current   |     -0.54 +/- 1.98     | -1.85e-05 +/- 6.85e-05 |  3.44e-03 +/- 5.6e-05  |  3.45e-03 +/- 3.9e-05  |
 test_objective_compute_atf              |     -0.47 +/- 2.20     | -4.76e-05 +/- 2.25e-04 |  1.02e-02 +/- 1.3e-04  |  1.02e-02 +/- 1.9e-04  |
 test_objective_jac_dshape_current       |     +0.83 +/- 9.98     | +3.28e-04 +/- 3.93e-03 |  3.97e-02 +/- 1.7e-03  |  3.94e-02 +/- 3.5e-03  |
 test_objective_jac_atf                  |     -5.65 +/- 2.59     | -1.12e-01 +/- 5.12e-02 |  1.87e+00 +/- 3.0e-02  |  1.98e+00 +/- 4.1e-02  |
 test_perturb_1                          |     -1.31 +/- 3.62     | -1.63e-01 +/- 4.50e-01 |  1.23e+01 +/- 3.0e-01  |  1.24e+01 +/- 3.4e-01  |
 test_perturb_2                          |     -3.01 +/- 2.78     | -5.46e-01 +/- 5.05e-01 |  1.76e+01 +/- 4.5e-01  |  1.81e+01 +/- 2.3e-01  |
 test_proximal_jac_atf                   |     -0.38 +/- 1.16     | -3.07e-02 +/- 9.46e-02 |  8.12e+00 +/- 6.5e-02  |  8.15e+00 +/- 6.9e-02  |
 test_proximal_freeb_compute             |     -0.34 +/- 1.25     | -6.19e-04 +/- 2.29e-03 |  1.82e-01 +/- 1.6e-03  |  1.83e-01 +/- 1.6e-03  |
 test_proximal_freeb_jac                 |     -0.79 +/- 1.49     | -5.93e-02 +/- 1.12e-01 |  7.42e+00 +/- 1.0e-01  |  7.48e+00 +/- 3.8e-02  |
 test_solve_fixed_iter                   |     -0.82 +/- 61.37    | -4.16e-02 +/- 3.10e+00 |  5.01e+00 +/- 2.2e+00  |  5.05e+00 +/- 2.2e+00  |

kianorr and others added 3 commits April 2, 2024 11:40
- added equal signs to ranges
- used jnp.where instead of directly slicing
Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Attention: Patch coverage is 98.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.36%. Comparing base (225d44e) to head (7060fb4).
Report is 24 commits behind head on master.

Files Patch % Lines
desc/geometry/curve.py 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #970      +/-   ##
==========================================
+ Coverage   95.33%   95.36%   +0.02%     
==========================================
  Files          90       90              
  Lines       22643    22741      +98     
==========================================
+ Hits        21586    21686     +100     
+ Misses       1057     1055       -2     
Files Coverage Δ
desc/coils.py 97.25% <100.00%> (ø)
desc/compute/_curve.py 100.00% <100.00%> (ø)
desc/geometry/curve.py 95.90% <93.75%> (-0.10%) ⬇️

... and 9 files with indirect coverage changes

kianorr and others added 2 commits April 4, 2024 22:34
- now works for batch arrays
- interpolate over whole array
desc/geometry/curve.py Outdated Show resolved Hide resolved
desc/compute/_curve.py Outdated Show resolved Hide resolved
desc/compute/_curve.py Outdated Show resolved Hide resolved
desc/compute/_curve.py Outdated Show resolved Hide resolved
desc/geometry/curve.py Outdated Show resolved Hide resolved
@kianorr
Copy link
Collaborator Author

kianorr commented Apr 17, 2024

  • I think the names for Xq, Yq, Zq should be more general so it's easier to copy-paste if we refactor something that applies to all of the compute function (although the bigger problem is that there is a bunch of repeated code, but not sure if that's fixable with compute functions).
  • there's a problem with having method in transforms since it's a string, but I'm still working on fixing that.

@kianorr
Copy link
Collaborator Author

kianorr commented Jul 16, 2024

My vectorization commits messed some stuff up so I think that explains the error in length. I'll have to look into it

image

image

@dpanici
Copy link
Collaborator

dpanici commented Jul 17, 2024

#1122

accidentally used full_f[istop], which just returned the last array, instead of full_f[:, istop]
when N = 1000 then the test fails for cubic and curvature(I think due to the differences within 2 knots for cubic interp)
@kianorr kianorr requested a review from f0uriest July 19, 2024 07:23
Copy link
Member

@f0uriest f0uriest left a comment

Choose a reason for hiding this comment

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

I think there might be an issue with CoilSets if there are spline curves that have different breakpoints. We only vmap over params, not transforms, since its assumed that every coil in a CoilSet can use the same transforms. I think this is also an issue if they have different knots after #1099

Possible fixes:

  • move knots and intervals back to params instead of transforms (not sure if this will work, what was the reason they were moved in the first place?)
  • Add logic to desc.coils._check_type to ensure that knots and intervals are the same for all SplineXYZCoil in a CoilSet

@kianorr
Copy link
Collaborator Author

kianorr commented Jul 19, 2024

I think there might be an issue with CoilSets if there are spline curves that have different breakpoints. We only vmap over params, not transforms, since its assumed that every coil in a CoilSet can use the same transforms. I think this is also an issue if they have different knots after #1099

Possible fixes:

  • move knots and intervals back to params instead of transforms (not sure if this will work, what was the reason they were moved in the first place?)
  • Add logic to desc.coils._check_type to ensure that knots and intervals are the same for all SplineXYZCoil in a CoilSet

I thought knots and intervals couldn't be in params because they're not optimizable?

desc/coils.py Outdated Show resolved Hide resolved
desc/coils.py Outdated Show resolved Hide resolved
desc/coils.py Outdated Show resolved Hide resolved
desc/coils.py Outdated Show resolved Hide resolved
desc/coils.py Outdated Show resolved Hide resolved
istop = jnp.where(istop == 0, -1, istop)

# fill f values outside of interval with break point values so that
# interpolation only takes into consideration the interval
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should note somewhere (maybe in a docstring) that the BC for a given spline is not enforced at the end of each interval but rather at the end of the 0 -> 2pi interval in s

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 docstring changes requested,
  • the basis kwarg is deprecated now, the compute functions should always return things in rpz (per Move basis change to main compute function #1027 )
  • since intervals is a new attribute of SplineXYZCurve/Coil, make sure in either set_up or wherever it needs to be (If forget exactly, search for like _rho or whatever for a FourierRZTorioidalSurface or rotmat for a Curve object, we do this for those), that intervals gets set to the correct value so that loading in old SplineXYZ objects does not cause issues where coil.intervals does not exist (just set to None is the thing to do)

desc/compute/_curve.py Outdated Show resolved Hide resolved
desc/compute/_curve.py Outdated Show resolved Hide resolved
desc/compute/_curve.py Outdated Show resolved Hide resolved
desc/compute/_curve.py Outdated Show resolved Hide resolved
desc/compute/_curve.py Outdated Show resolved Hide resolved
desc/compute/_curve.py Outdated Show resolved Hide resolved
desc/compute/_curve.py Outdated Show resolved Hide resolved
desc/geometry/curve.py Outdated Show resolved Hide resolved
test("linear", "torsion", compare_breaks=False)

test("cubic", "length")
# don't include break points because of interpolator BCs
Copy link
Collaborator

Choose a reason for hiding this comment

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

re-think of why this fails, probably not due to the BC but the fact that the BC of the interval (besides at 0 and 2pi) is determined by whatever the spline does when it encounters the flat regions at the edges of the specific interval being considered

@f0uriest f0uriest mentioned this pull request Jul 23, 2024
@kianorr
Copy link
Collaborator Author

kianorr commented Aug 27, 2024

image image

The first grid exactly doubles the xyz coordinates of the query point at pi compared to the second grid.

@dpanici
Copy link
Collaborator

dpanici commented Aug 28, 2024

check if evaluating a single point at the halfway pt yields the same error as the full grid evaluation

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.

4 participants