Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SurfaceRZFourier cache isn't invalidated by setting rc, zs, rs, zc arrays #465

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

missing-user
Copy link
Contributor

There are not setter properties for the variables rc, zs, rs, zc, so recompute_bell() is not triggered by overwriting them.

For example, this means that plots, or depending objective functions aren't updated when overwriting the entire rc array, e.g. to scale a configuration by a constant factor. The following example script will plot the same torus twice, eventho the coefficients were changed (same problem with ).

from simsopt import geo
import matplotlib.pyplot as plt
s = geo.SurfaceRZFourier(3, True, 5, 8)
s.make_rotating_ellipse(1, .2, .5, .1)
s.plot()
plt.show()
scaling = 10
s.rc = s.rc * scaling # This doesn't invalidate the cache
s.zc *= scaling         # or equivalently
s.plot()
plt.show()

can now be done with

from simsopt import geo
import matplotlib.pyplot as plt
s = geo.SurfaceRZFourier(3, True, 5, 8)
s.make_rotating_ellipse(1, .2, .5, .1)
s.plot()
plt.show()
scaling = 10
s.rc_array = s.rc * scaling
s.zc_array *= scaling
s.plot()
plt.show()

My suggestion is to either

  • Add array setter properties to the fourier components of the surface (this PR)
  • Refactor SurfaceRZFourier to have private _rc and property decorators for the accessors rc, but that would also involve changing the CPP class and seems quite invasive
  • Add a short warning to the set_zs() docstring:
    """Modifyting the zs array directly is discouraged, since it doesn't trigger the recompute_bell(). """

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant