hiddenSymmetries / simsopt

Simons Stellarator Optimizer Code
https://simsopt.readthedocs.io
MIT License
83 stars 43 forks source link

Mgrid improvements #396

Closed aaroncbader closed 3 months ago

aaroncbader commented 4 months ago

This PR updates the mgrid writing script to also include the arrays for the Magnetic Potential. These values are needed in some codes (such as BMW).

There is a separate issue in the mgrid to allow for calculation of multiple coil sets separately with scaled currents, but that requires some more thought.

mbkumar commented 4 months ago

In the MGrid class, the method from_file should read the A_vec data as well.

landreman commented 4 months ago

I've never seen mgrid files that contain the vector potential before. Is there some version of xgrid other than the one in stellopt that writes mgrid files containing A? mgrid files are already quite large and storing A makes them twice as big, and vmec doesn't need A, so I'm not sure it makes sense to always include A.

aaroncbader commented 3 months ago

I've never seen mgrid files that contain the vector potential before. Is there some version of xgrid other than the one in stellopt that writes mgrid files containing A? mgrid files are already quite large and storing A makes them twice as big, and vmec doesn't need A, so I'm not sure it makes sense to always include A.

The oak ridge version of MakeGrid includes the vector potential. It is necessary to run BMW.

aaroncbader commented 3 months ago

Will make including potential optional and default to off

aaroncbader commented 3 months ago

I would like to update the test that reads an mgrid file to include one with the potential fields enabled. But I don't know how to regenerate the test mgrid file.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 87.50000% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 91.17%. Comparing base (1fb6e87) to head (377a793).

Files Patch % Lines
src/simsopt/field/mgrid.py 85.48% 9 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #396 +/- ## ========================================== - Coverage 91.56% 91.17% -0.39% ========================================== Files 74 74 Lines 12762 12829 +67 ========================================== + Hits 11685 11697 +12 - Misses 1077 1132 +55 ``` | [Flag](https://app.codecov.io/gh/hiddenSymmetries/simsopt/pull/396/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hiddenSymmetries) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/hiddenSymmetries/simsopt/pull/396/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hiddenSymmetries) | `91.17% <87.50%> (-0.39%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hiddenSymmetries#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

landreman commented 3 months ago

It looks like that test file mgrid.pnas-qa-test-lowres-standard.nc was added by Tony Qian, so you could ask him. Or just use a different file for testing mgrid files that contain A.

aaroncbader commented 3 months ago

Should I be worried about the failed tests?

mbkumar commented 3 months ago

@aaroncbader The issue is not with your tests, but one of Spec tests is failing. I am not sure if that test is using the mgrid file that was modified, but I doubt it.

aaroncbader commented 3 months ago

There looks to be an issue with loading mgrids. But it's not one that I introduced. Simsopt is unable to load mgrids that it saves. It looks like this problem predates my changes. I'll see if I can fix it.

landreman commented 3 months ago

Should I be worried about the failed tests?

No need to worry - these failures are a known problem, #357, which is independent of this PR. @abaillod @smiet Any hope of fixing #357?

aaroncbader commented 3 months ago

ok, I've fixed up the mgrid from_file function and added a test for a new mgrid with a potential value