Cbs/change resolution fix #519
Conversation
Changing the resolution would lead to segmentation fault because Cpp side memory was not updated. Fixing this led to pybind11 issues since pybind11 interface does not like changes in returned array size. Just make a new surface, why try to do something so complicated?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #519 +/- ##
==========================================
- Coverage 92.01% 92.00% -0.01%
==========================================
Files 82 82
Lines 15993 15976 -17
==========================================
- Hits 14716 14699 -17
Misses 1277 1277
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:
|
| will have a magnitude of zero. Any previous nonzero Fourier | ||
| amplitudes that are not within the new range will be | ||
| discarded. | ||
| return a new surface with Fourier resolution mpol, ntor |
There was a problem hiding this comment.
In the interest of better docstrings throughout the code, please add better docstrings here and elsewhere in SurfaceRZFourier, including Input and Return parameters and their types/shapes.
|
|
||
| # Update the dofs object | ||
| self.replace_dofs(DOFs(self.get_dofs(), self._make_names())) | ||
| return self.copy(mpol=mpol, ntor=ntor) |
There was a problem hiding this comment.
It looks like you no longer truncate the amplitudes not within the new range:
if mpol < old_mpol or ntor < old_ntor:
self.invalidate_cache()
How is chopping these lines changing the calculation?
| a2 = s.area() | ||
| s3 = s2.change_resolution(mpol+1, ntor+1) | ||
| v2 = s3.volume() | ||
| a2 = s3.area() |
There was a problem hiding this comment.
Can you add a test here that checks if it fixes Issue #478? e.g. tests that s.darea_by_dcoeff() and other surface methods can be called after changing the resolution?
There was a problem hiding this comment.
When we update the boundary for an MHD object with a new Surface object, what happens to the old Surface object? We need to be a bit careful here. Should we explicitly delete the old object and remove all the links between the old Surface object and the parent and child Optimizable objects connected to the old Surface object?
| if not surf.stellsym and not self.stellsym: | ||
| surf.set_zc(m, n, self.get_zc(m, n)) | ||
| surf.set_rs(m, n, self.get_rs(m, n)) | ||
|
|
There was a problem hiding this comment.
Instead of looping, can't you use index slicing and copy. Could be faster.
| # Expand number of Fourier modes to include larger poloidal mode numbers: | ||
| s.boundary.change_resolution(6, s.boundary.ntor) | ||
| s.boundary = s.boundary.change_resolution(6, s.boundary.ntor) | ||
| # To make this example run relatively quickly, we will optimize in a |
There was a problem hiding this comment.
What happens to the original boundary? Can you print the Optimizable graph and ensure that the old one is garbage collected (or atleast not used).
3 participants
This adapts the change_resolution to return a copy of the surface, fixes the issue with the cpp backend #478, and aligns it with the return signature of
SurfaceRZPseudospectral.change_resolution.