Multifilament coil approximation #223
Conversation
Codecov Report
@@ Coverage Diff @@
## master #223 +/- ##
==========================================
+ Coverage 90.83% 91.01% +0.18%
==========================================
Files 59 60 +1
Lines 7733 7899 +166
==========================================
+ Hits 7024 7189 +165
- Misses 709 710 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for contributing this!
codecov is flagging a few cases and functions in curvefilament.py that are not covered. You might see if a few of these cases can be covered without much extra work.
Consider moving stage_two_optimization_finitebuild.py to the 3_Advanced subdirectory?
| @@ -0,0 +1,151 @@ | |||
| #!/usr/bin/env python | |||
| r""" | |||
| In this example we solve a FOCUS like Stage II coil optimisation problem: the | |||
There was a problem hiding this comment.
In this docstring, how about saying something about finite build, discuss how the parameter space for this optimization includes the Fourier modes of alpha(theta), and compare and contrast this example with stage_two_optimization.py. It would also be nice to add a reference to the Singh paper here (I see there is a reference in the source, but one here too would be useful to help explain how this example works.)
| numfilaments_n = 1 | ||
| numfilaments_b = 5 | ||
| gapsize_n = 0.01 | ||
| gapsize_b = 0.01 |
There was a problem hiding this comment.
How about moving these 4 parameters from lines 74-77 up to around line 52 so they are with the other parameters. Also it would be helpful to explain in comments what _n and _b and gapsize mean.
|
|
||
|
|
||
| base_curves_finite_build = sum([ | ||
| create_multifilament_grid(c, numfilaments_n, numfilaments_b, gapsize_n, gapsize_b, rotation_order=3) for c in base_curves], []) |
There was a problem hiding this comment.
How about making rotation_order one of the parameters as in lines 30-54, so users have an easier time spotting how to control this aspect of the parameter space.
| # Define the objective function: | ||
| Jf = SquaredFlux(s, bs) | ||
| Jls = [CurveLength(c) for c in base_curves] | ||
| curves_for_dist = [c.curve for c in coils[::(numfilaments_n*numfilaments_b)]] |
There was a problem hiding this comment.
Worth a comment about what is going on here.
| @@ -72,6 +72,31 @@ def __neg__(self): | |||
| return ScaledCurrent(self, -1.) | |||
|
|
|||
|
|
|||
| def apply_symmetries_to_curves(base_curves, nfp, stellsym): | |||
tests/geo/test_curvefilament.py
Outdated
| fils = create_multifilament_grid( | ||
| c, numfilaments_n, numfilaments_b, gapsize_n, gapsize_b, | ||
| rotation_order=3, rotation_scaling=None) | ||
|
|
||
| xr = fils[0].rotation.x | ||
| fils[0].rotation.x = xr + 1e-2*np.random.standard_normal(size=xr.shape) |
There was a problem hiding this comment.
Do these lines after line 110 do anything?
tests/geo/test_curvefilament.py
Outdated
| fils = create_multifilament_grid( | ||
| c, numfilaments_n, numfilaments_b, gapsize_n, gapsize_b, | ||
| rotation_order=3, rotation_scaling=None) |
There was a problem hiding this comment.
Are these lines unnecessary?
tests/geo/test_curvefilament.py
Outdated
|
|
||
|
|
||
| class MultifilamentTesting(unittest.TestCase): | ||
|
|
There was a problem hiding this comment.
Would it be worth adding a test case in this file which is basically the Taylor test in stage_two_optimization_finitebuild.py but with asserts? If the Taylor test in that example fails, presently no error is raised. The Taylor test in that example ensures that the finite build machinery works even when applying nfp-symmetry and stellarator-symmetry and plugging the finite-build coils into Biot-Savart, and it's not obvious that the other tests in test_curvefilament.py cover those aspects.
| base_currents[0].fix_all() | ||
|
|
||
|
|
||
| base_curves_finite_build = sum([ |
There was a problem hiding this comment.
Here and in line 88, the sum is not obvious. What does it mean to sum two curves? I guess it is list concatenation? Can you add a comment about what the sum does in these contexts?
src/simsopt/geo/curvefilament.py
Outdated
|
|
||
|
|
||
| class ZeroRotation(Optimizable): | ||
|
|
|
@landreman I think I have addressed all of your comments. Can you please have a look again? |
Adds the approach presented in Singh et al to approximate finite-build coils using multiple filaments.