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

[VMEC,SurfaceRZFourier] fix inconsistency regarding mpol #437

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

Conversation

jons-pf
Copy link
Contributor

@jons-pf jons-pf commented Jul 17, 2024

VMEC only ever uses poloidal modes up to m=mpol-1 with mpol being specified in the VMEC input. SurfaceRZFourier calls their poloidal Fourier resolution also mpol, but includes poloidal modes including up to m=mpol. This leads to inconsistencies when converting between VMEC input/output files and SurfaceRZFourier. This PR addresses this inconsistency.

@eguiraud-pf
Copy link

eguiraud-pf commented Jul 17, 2024

Hi, to add to @jons-pf 's comment, some examples of inconsistencies. When the indata file contains RBC/ZBS Fourier coefficients for m == mpol (which VMEC ignores but SurfaceRZFourier includes):

  • Vmec._boundary does not reflect exactly what VMEC uses as boundary condition
  • similarly SurfaceRZFourier.from_vmec_input(indata_file) does not correspond exactly to the boundary VMEC optimizes for

# VMEC only considers poloidal modes up to `m=(mpol-1)`,
# but `SurfaceRZFourier` includes `m=mpol`.
# Here, `xm` comes from VMEC and `mpol` goes into `SurfaceRZFourier`.
mpol = int(np.max(xm)) - 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this -1 should be here. mpol here is SurfaceRZFourier's mpol, so it should equal the maximum m.

for m in range(mpol_capped + 1):
# The highest mode number that VMEC can use is `m=100`
# if the max resolution is `mpol=101`.
for m in range(mpol_capped):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem correct to remove the +1 here. mpol_capped is essentially mpol of a SurfaceRZFourier object (via boundary_RZFourier = self.boundary.to_RZFourier() and mpol_capped = np.min([boundary_RZFourier.mpol, 101]). So the m=mpol mode should be included rather than excluded. If you want to change the 101 to 100 on line 523/528, that would be fine.

@jons-pf
Copy link
Contributor Author

jons-pf commented Oct 9, 2024

Just to give a signal of life here - I have not forgotten about this issue, but sadly have not found time yet to continue digging into this.

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.

4 participants