SurfaceRZFourier conversion to Desc format#628
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds bidirectional conversion between SurfaceRZFourier and DESC’s FourierRZToroidalSurface, with a roundtrip unit test to validate geometry preservation.
Changes:
- Implement
SurfaceRZFourier.from_desc(...)to build a Simsopt surface from a DESC surface. - Implement
SurfaceRZFourier.to_desc()to export a Simsopt surface to DESC. - Add a DESC-enabled roundtrip test using VMEC input files.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/simsopt/geo/surfacerzfourier.py |
Adds to_desc / from_desc conversion logic and DESC imports/guards. |
tests/geo/test_surface_rzfourier.py |
Adds a DESC roundtrip test to validate conversion preserves geometry and metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| np.testing.assert_allclose(boundary_from_desc.rs, surface_orig.rs, atol=1e-12) | ||
| np.testing.assert_allclose(boundary_from_desc.zc, surface_orig.zc, atol=1e-12) |
| try: | ||
| from desc.geometry import FourierRZToroidalSurface as DescFourierRZToroidalSurface | ||
| from desc import vmec_utils as desc_vmec_utils | ||
| except ImportError: | ||
| DescFourierRZToroidalSurface = None |
| @classmethod | ||
| @SimsoptRequires(DescFourierRZToroidalSurface is not None, "from_desc method requires Desc module") | ||
| def from_desc(cls, surface: DescFourierRZToroidalSurface): |
| @SimsoptRequires(DescFourierRZToroidalSurface is not None, "to_desc method requires Desc module") | ||
| def to_desc(self) -> DescFourierRZToroidalSurface: |
| return DescFourierRZToroidalSurface( | ||
| R_lmn=r_lmn.flatten(), | ||
| Z_lmn=z_lmn.flatten(), | ||
| modes_R=modes, | ||
| modes_Z=modes, | ||
| NFP=n_field_periods, | ||
| sym=is_stellarator_symmetric, | ||
| check_orientation=False, | ||
| ) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #628 +/- ##
==========================================
+ Coverage 90.24% 90.32% +0.07%
==========================================
Files 84 84
Lines 17896 17950 +54
==========================================
+ Hits 16150 16213 +63
+ Misses 1746 1737 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| - name: Install virtual_casing | ||
| run: pip install -v git+https://github.com/hiddenSymmetries/virtual-casing | ||
|
|
||
| - name: Install desc-opt |
There was a problem hiding this comment.
also add to extensive tests
| surface_orig = SurfaceRZFourier(mpol=2, ntor=0, nfp=1, stellsym=True) | ||
| surface_orig.set_rc(0, 0, 1.0) | ||
| surface_orig.set_rc(1, 0, 0.1) | ||
| surface_orig.set_zs(1, 0, 0.1) |
There was a problem hiding this comment.
How about replacing 0.1 here with a different number since rc(1,0) is also 0.1, just to cover the case in which rc and zs were to get swapped somehow.
| # check_orientation=False prevents DESC from flipping Z_lmn sign. | ||
| desc_surface = DescFourierRZToroidalSurface( | ||
| R_lmn=np.array([1.0, 0.1]), | ||
| Z_lmn=np.array([0.1]), |
There was a problem hiding this comment.
How about replacing 0.1 here with another value, for the same reason as above
| @unittest.skipIf(DescFourierRZToroidalSurface is None, "desc python extension is not installed") | ||
| def test_to_from_desc_roundtrip(self): | ||
| """Test that to_desc and from_desc correctly converts DESC surface back to simsopt.""" | ||
| import os |
There was a problem hiding this comment.
Move import os to top of file
| filelist = ["input.rotating_ellipse", 'input.LandremanPaul2021_QH_reactorScale_lowres', "input.ITERModel", "input.li383_low_res", "input.basic_non_stellsym"] | ||
|
|
||
| for ff in filelist: | ||
| # input_file = str(TEST_DIR / 'input.LandremanPaul2021_QH_reactorScale_lowres') |
| gamma_flat = surface_orig.gamma().reshape((-1, 3)) | ||
| gamma_rt_flat = boundary_from_desc.gamma().reshape((-1, 3)) | ||
| gamma_err = np.max(np.linalg.norm(gamma_flat - gamma_rt_flat, axis=-1)) | ||
| self.assertAlmostEqual(gamma_err, 0.0, places=12) |
There was a problem hiding this comment.
In place of these 4 lines it would be cleaner to use np.testing.assert_allclose(surface_orig.gamma(), boundary_from_desc.gamma())
| return simsopt_surface | ||
|
|
||
| @SimsoptRequires(DescFourierRZToroidalSurface is not None, "to_desc method requires Desc module") | ||
| def to_desc(self) -> DescFourierRZToroidalSurface: |
There was a problem hiding this comment.
How about add check_orientation=True as a kwarg to this method here that gets passed to line 638, so users have more control over whether the orientation is flipped or not.
Summary
This PR introduces new methods to convert
SurfaceRZFourierobjects to and from the equivalent Desc format,FourierRZToroidalSurface. The methods are covered by unit tests which convert stellarator-symmetric and non-stellarator symmetric surfaces.New Methods
Two new methods have been added to the
SurfaceRZFourierclass:to_desc: converts an instance of a surface to the Desc format,from_desc: a class method that builds aSurfaceRZFourierobject from an existing DescFourierRZToroidalSurface.