-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add compute implementation for surface geometry terms #884
Conversation
| benchmark_name | dt(%) | dt(s) | t_new(s) | t_old(s) |
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
test_build_transform_fft_lowres | -7.83 +/- 2.88 | -1.06e-03 +/- 3.91e-04 | 1.25e-02 +/- 8.5e-05 | 1.36e-02 +/- 3.8e-04 |
test_build_transform_fft_midres | -4.24 +/- 1.53 | -4.07e-03 +/- 1.46e-03 | 9.19e-02 +/- 1.2e-03 | 9.59e-02 +/- 8.6e-04 |
test_build_transform_fft_highres | -1.30 +/- 0.96 | -6.11e-03 +/- 4.52e-03 | 4.62e-01 +/- 3.0e-03 | 4.69e-01 +/- 3.4e-03 |
test_equilibrium_init_lowres | +1.83 +/- 2.92 | +1.10e-02 +/- 1.75e-02 | 6.12e-01 +/- 1.5e-02 | 6.01e-01 +/- 9.0e-03 |
test_equilibrium_init_medres | -0.08 +/- 2.21 | -9.91e-04 +/- 2.78e-02 | 1.26e+00 +/- 2.0e-02 | 1.26e+00 +/- 2.0e-02 |
test_equilibrium_init_highres | +0.16 +/- 0.96 | +6.47e-03 +/- 4.02e-02 | 4.17e+00 +/- 3.2e-02 | 4.17e+00 +/- 2.4e-02 |
test_objective_compile_dshape_current | +3.49 +/- 11.20 | +1.53e-01 +/- 4.90e-01 | 4.52e+00 +/- 3.2e-01 | 4.37e+00 +/- 3.7e-01 |
test_objective_compile_atf | +0.45 +/- 10.83 | +4.26e-02 +/- 1.02e+00 | 9.49e+00 +/- 8.4e-01 | 9.45e+00 +/- 5.8e-01 |
test_objective_compute_dshape_current | +2.87 +/- 2.67 | +6.18e-05 +/- 5.73e-05 | 2.21e-03 +/- 4.8e-05 | 2.15e-03 +/- 3.2e-05 |
test_objective_compute_atf | -0.05 +/- 2.66 | -3.98e-06 +/- 2.04e-04 | 7.66e-03 +/- 6.7e-05 | 7.66e-03 +/- 1.9e-04 |
test_objective_jac_dshape_current | -2.59 +/- 14.48 | -1.20e-03 +/- 6.70e-03 | 4.50e-02 +/- 4.3e-03 | 4.62e-02 +/- 5.1e-03 |
test_objective_jac_atf | +1.13 +/- 4.09 | +2.54e-02 +/- 9.19e-02 | 2.28e+00 +/- 8.0e-02 | 2.25e+00 +/- 4.6e-02 |
test_perturb_1 | -0.09 +/- 16.02 | -8.05e-03 +/- 1.40e+00 | 8.75e+00 +/- 9.6e-01 | 8.76e+00 +/- 1.0e+00 |
test_perturb_2 | -0.08 +/- 3.92 | -1.23e-02 +/- 5.85e-01 | 1.49e+01 +/- 2.9e-01 | 1.49e+01 +/- 5.1e-01 | |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #884 +/- ##
==========================================
- Coverage 94.91% 94.91% -0.01%
==========================================
Files 80 80
Lines 19947 19966 +19
==========================================
+ Hits 18933 18951 +18
- Misses 1014 1015 +1
|
) | ||
def _a_major_over_a_minor(params, transforms, profiles, data, **kwargs): | ||
A = transforms["grid"].compress(data["A(z)"], surface_label="zeta") | ||
P = transforms["grid"].compress(data["perimeter(z)"], surface_label="zeta") | ||
# derived from Ramanujan approximation for the perimeter of an ellipse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this always work? What is the shape is like a triangle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's always an approximation for any non-elliptical cross section, since the definitions of major and minor axes and elongation aren't uniquely defined in those cases. In practice it still gives reasonable results in all the cases we've tried it. For a triangular cross section you'd basically be getting the elongation of an equivalent elipse which would likely be close to 1, which is what we'd expect.
x = eq.compute(key)[key].max() # max needed for elongation broadcasting | ||
y = eq.surface.compute(key)[key].max() | ||
if key == "a_major/a_minor": | ||
rtol, atol = 1e-2, 0 # need looser tol here bc of different grids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I increase the resolution of both the grids, can I reduce rtol and still pass the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in theory yes. The main difference is that the eq.compute uses a quadrature grid to compute the perimeter, which doesn't include nodes at rho=1, so we extrapolate what the true perimeter is based on an approximation from rho~0.9x. As you increase the resolution, the outermost node in the quadrature grid will approach rho=1 but you'd need to go to really high resolution.
FourierRZToroidalSurface
Elongation
andAspect
ratio objectives to target surfaces as well as equilibriaResolves #795