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

Mgrid improvements #396

Merged
merged 9 commits into from
Apr 7, 2024
Merged

Mgrid improvements #396

merged 9 commits into from
Apr 7, 2024

Conversation

aaroncbader
Copy link
Contributor

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
Copy link
Collaborator

mbkumar commented Feb 27, 2024

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

@landreman
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Will make including potential optional and default to off

@aaroncbader
Copy link
Contributor Author

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.

Copy link

codecov bot commented Apr 4, 2024

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 ⚠️
Additional details and impacted files
@@            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 Coverage Δ
unittests 91.17% <87.50%> (-0.39%) ⬇️

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

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

@landreman
Copy link
Contributor

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
Copy link
Contributor Author

Should I be worried about the failed tests?

@mbkumar
Copy link
Collaborator

mbkumar commented Apr 5, 2024

@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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@landreman landreman merged commit b287714 into master Apr 7, 2024
42 of 47 checks passed
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.

3 participants