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

Updated mp00ac.f90 to read and output gauge invariant helicity. #187

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

Conversation

KseniaAleynikova
Copy link
Collaborator

I believe this should be a standard way of working with helicity in SPEC that is why I do not use an extra flag with/without gauge invariant helicity.
But tests should be done first.

…ieve this should be a standard way of working with helicity in SPEC that is why I do not use an extra flag with/without gauge invariant helicity.
@jloizu
Copy link
Collaborator

jloizu commented Mar 29, 2023

Great, Ksenia. My initial comments are:

  • the CI tests don't seem to pass, maybe you can check why is that? It should work.
  • maybe it would be good to write down in a couple of equations (ideally, put this into the SPEC source using doxygen convetion, so that it appears in the documentation) exactly what you implemented.
  • It makes me think that we don't have any CI test (even in slab) at fixed helicity (right?), maybe we should create one reference case that runs with the master branch (e.g. take one of the cases with fixed mu, then extract K and run it with fixed K, but with initial geometry not in equilibrium, to make sure things move correctly). Then we can test your new version with that test case as well, and one should get the same.
  • Maybe you could also add one of your w7x fixed-helicity runs from your paper? That could be in a new folder of input files (not test cases, not verification, but e.g. "production")? We can use that as well as test for the new version?

@smiet
Copy link
Collaborator

smiet commented Oct 16, 2023

Just had a look at the test logs (on the updated ci). The test run_fast_cylinder fails because of an out-of-bound index access in the solution-array.

Error termination. Backtrace:
At line 1067 of file /home/runner/work/SPEC/build/src/mp00ac_m.F90
Fortran runtime error: Index '0' of dimension 1 of array 'solution' below lower bound of 1

Despite this, the CI will probably still fail since the value of 'Helicity' in the outputs will be different from that in the pre-computed files that it is checked against.

For this change to be accepted we need to generate new test files with the correct value of Helicity using this convention.
For this you should do the following steps:

  • run the new xspec on all the files in ${SPEC_ROOT}/ci/${TEST_NAME}
  • test if the value of Helicity is the only difference by running python -m py_spec.ci.test compare.h5 ${TEST_NAME}.h5
    • If the command fails, you need to re-install py_spec [cd ${SPEC_ROOT}/Utilities/pythontools && pip install -e .] since Antoine and I just fixed a bug regarding this.
  • if that is the only difference, remove the old compare.h5, and rename the new .h5 file you created `compare.h5'
  • add all the new files to the repo and see the tests pass.

@zhucaoxiang, @jloizu, @abaillod, could you comment on which Helicity convention is best convention and why?

@smiet
Copy link
Collaborator

smiet commented Dec 1, 2024

@missing-user helpfully fixed the failing case, Thanks Phillip!

This needs a bit of discussion before we accept this change as outputs will change: @jloizu, @ErolBa @SRHudson, @zhucaoxiang what do you think? It has my vote!

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.

6 participants