surface copying classmethod

[smiet]

Added a classmethod to surfacerzfourier.py that copies an existing surface but allows the user to request a different nphi, ntheta and grid_range.

Usecase: your existing equilibrium has half period, but you want to plot the full surface, so you use this method to generate a brother with higher resolution and full range.

Currently only implemented surfaceRZFourier, if this is deemed useful, will generate for the other classes as well.

[is a bit of duplication when this is added to each inherited class, but cleaner than adding a classmethod to the Surface class, as different arrays need to be populated in each inherited class's constructor, which does not follow a neat pattern]

If useful, give me some time to add classmethod to other classes, otherwise please cancel request. If I'm duplicating functionality, my apoligies, Thanks!

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04% 🎉

Comparison is base (f8f9d59) 91.07% compared to head (2cbe433) 91.12%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #348      +/-   ##
==========================================
+ Coverage   91.07%   91.12%   +0.04%     
==========================================
  Files          70       70              
  Lines       12367    12391      +24     
==========================================
+ Hits        11263    11291      +28     
+ Misses       1104     1100       -4     

Flag	Coverage Δ
unittests	91.12% <100.00%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/simsopt/geo/surfacerzfourier.py	95.19% <100.00%> (+1.08%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[smiet]

Does anyone know why the unit tests are failing? The code I submitted does not affect the tests that fail...

[landreman]

Does anyone know why the unit tests are failing? The code I submitted does not affect the tests that fail...

Weird, I don't know why those tests would fail, given the changes. I believe the failing CurvePerturbationTesting tests involve random numbers so perhaps something is not deterministic, but that wouldn't explain why we haven't experienced this issue before. @mbkumar any ideas?

[andrewgiuliani]

This functionality is already implemented in the shared dofs feature. If you want a new SurfaceRZFourier with the same dofs as another, but different set of quadrature points, see the following unit test:

simsopt/tests/geo/test_surface_rzfourier.py

Line 60 in c859dfb

def test_shared_dof_init(self):

[smiet]

@andrewgiuliani I intended this method to provide different functionality from the example; the dofs are not shared, but a full copy is made.

This would allow the user also for example make copies of the surface object during optimization, and plot a series of surfaces, in the same plot, or next to each other, and to 'save' the initial or intermediate state for comparison with the final result.

Having it as a class method is also a nice convenience for the user, who does not have to look into the class definition.
If it is only me who likes this way of working, let me know and I will close the pull request.

[landreman]

Generally I think it does seem useful to include this kind of copy function, for the use cases @smiet mentioned, which are a bit different from the use cases for shared dofs.

In order to generalize this routine to other Surface types, maybe there is a clever way to do it by serializing the original surface and then deserializing?

Presently the following test fails, specifying only ntheta:

s2 = SurfaceRZFourier.from_other_surface(s, ntheta=100)
self.assertEqual(s2.quadpoints_theta.size, 100)

Result:

AssertionError: 62 != 100

Do we want that to work? Or should a user need to also specify nphi and range in order to change only ntheta?

If you didn't already see this, running ./run_autopep from the root directory will fix any linting errors.

[andrewgiuliani]

If you want to get this working on SurfaceXYZFourier and SurfaceXYZTensorFourier, I can suggest a more general approach since the user is allowed to change mpol, ntor, quadpoints, stellsym, etc... One possibility that is agnostic of the surface type is to evaluate the XYZ positions on the old surface and complete a least squares projection of those points onto the new surface basis functions using least_squares_fit.

simsopt/src/simsoptpp/surface.cpp

Line 40 in f8f9d59

void Surface<Array>::least_squares_fit(Array& target_values) {

You'd just need to make sure there are enough quadrature points in the projection so that the normal equations aren't singular.

Alternatively, you might want to hard code a change_resolution function for SurfaceXYZFourier and SurfaceXYZTensorFourier and you'll have to go into the weeds of their DOF orderings.

[mbkumar]

Instead of having a class method, why don't you have a new method called copy? The intent will be clear. With the copy method, you can generate an instance of the same class as that of the original object. And the expectation would also be the same.

[smiet]

@mbkumar this is the way, and this is why we have code review. I will implement the change, update the pull request and re-factor the code I have written using my convoluded method in the coming week(s)