Single-stage optimization with spec: CoilSet, ReducedCoilSet and CoilNormalField (PR 2 of 2) #438
Conversation
smiet
requested review from
landreman,
rogeriojorge,
abaillod and
andrewgiuliani
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #438 +/- ##
==========================================
- Coverage 92.68% 92.34% -0.34%
==========================================
Files 77 78 +1
Lines 13982 14503 +521
==========================================
+ Hits 12959 13393 +434
- Misses 1023 1110 +87
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -421,6 +421,75 @@ def from_pyQSC(cls, stel: Qsc, r: float = 0.1, ntheta=20, mpol=10, ntor=20, **kw | |||
|
|
|||
| surf.local_full_x = surf.get_dofs() | |||
| return surf | |||
|
|
|||
| def copy(self, **kwargs): | |||
There was a problem hiding this comment.
What is the difference between this function and simply setting the dofs of a new surface object as, for example
surf = SurfaceRZFourier.from_vmec_input(filename, range="half period", nphi=nphi, ntheta=ntheta) surf_big = SurfaceRZFourier(dofs=surf.dofs,nfp=nfp, mpol=surf.mpol,ntor=surf.ntor, quadpoints_phi=new_quadpoints_phi,quadpoints_theta=new_quadpoints_theta)
There was a problem hiding this comment.
The functionality is identical; except that one needs to know how the surface was instantiated in order to produce a copy with different quadpoints, mpol, ntor, or another quantity.
My CoilSet and NormalField need to a Surface of type SurfaceRZFourier with quadpoints of a specificrange, but a general Surface object does not carry with it the information of from which filename it was instantiated, or other information to generate a new surface in the way you demonstrate above. The .copy method allows me to generate a copy that is otherwise identical to the surface it copies, except for the kwargs you have specified.
| @@ -1,9 +1,8 @@ | |||
| #!/usr/bin/env python | |||
|
|
|||
| import unittest | |||
| import os | |||
There was a problem hiding this comment.
Did os fail in same particular case? Is it why it was removed?
There was a problem hiding this comment.
most tests use monty.ScratchDir but the test_mgrid used tempfile and os.chdir, without but did not contain a chdir out of the temporary directory.
This led my subsequent tests to fail mysteriously as the temporary directory that the test-kernel had moved into was deleted in when the test finished.
Reverted this test to the more readable and less error-prone moty.ScratchDir, that uses the with context and therefore os import was also no longer needed to effect the chdir, so I removed it.
| @@ -771,6 +771,24 @@ def test_fourier_transform_scalar(self): | |||
| # Check that the result is the same as the original field: | |||
| np.testing.assert_allclose(field/2*np.pi**2, field2) | |||
|
|
|||
| def test_copy_method(self): | |||
There was a problem hiding this comment.
Some lines of copy are not being tested (see codecov)
There was a problem hiding this comment.
Should be in the latest push, with a logic fix!
| @@ -705,7 +774,7 @@ def fourier_transform_scalar(self, scalar, mpol=None, ntor=None, normalization=N | |||
| if not stellsym: | |||
| A_mnc[0, ntor] = np.sum(scalar) / (ntheta_grid * nphi_grid) | |||
| if normalization is not None: | |||
| if isinstance(normalization, float): | |||
| if not isinstance(normalization, float): | |||
There was a problem hiding this comment.
I'd be much happier if the mistake weren't my own ;)
|
Sorry to push this, but could we move forward with this PR? All tests are now pasing (after the numpy2.0 speedbump), and the tests now cover all new code. The only exception is a plotting utility function that I am not testing because not all runners have visualization capabilities, but I do think it's inclusion is valuable as a demonstration. |
|
Looks good to me. However, I'll let a day or two go by before accepting in case @landreman or @abaillod wants to add something here. |
|
There's a conflict with the recent merge. Could you fix it @smiet? I'll merge in afterwords. |
|
Merged |
This PR adds new features, and enables the optimizations presented at the Simons' retreat and meeting, and available in preprint (https://arxiv.org/abs/2407.02097).
The main additions are:
class
CoilSetA helper class to handle a set of
coilobjects as a single object:ReducedCoilSetReducedCoilSetandCoilNormalFieldto be implemented with minimal code duplication.class
ReducedCoilSetIdentical in use to
CoilSet, but it's degrees-of-freedom are linear combinations of the coil degrees-of-freedom.class
CoilNormalFieldIdentical in use to a NormalField and interfaces with SPEC, but has degrees-of-freedom specified by a
CoilSetorReducedCoilSet.CoilSetandReducedCoilSet, provides method to reduce theCoilSetusing the mapping to SPEC input parameters (normal field Fourier components).With these changes the functionality I describe in my preprint will be added to the simsopt master branch. I look forward to your suggestions to improve my code.