gafusion / omas

Ordered Multidimensional Array Structure
http://gafusion.github.io/omas
MIT License
32 stars 15 forks source link

Cherry pick omas viewer dev #256

Closed AreWeDreaming closed 6 months ago

AreWeDreaming commented 1 year ago

This cherry picked the changes from omas_viewer_dev.

Ready for review

AreWeDreaming commented 1 year ago

The regression test fails because of errors that are likely in OMFIT:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
TestOmasExamples.test_plot_omas_omfit
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Possible problems found with gacode profiles file:
 * pow_e_line is negative (should always be positive)
make: *** [Makefile:26: test] Error 1
Error: Process completed with exit code 2.

What OMFIT version does the test environment use?

AreWeDreaming commented 1 year ago

This has been tested as part of #257 and seems to work for that case.

github-actions[bot] commented 1 year ago

Stale pull request message

smithsp commented 11 months ago

I tried following a series of PRs here on omas that all depend on each other. Can @AreWeDreaming open an issue here on omas with the correct order of operations for merging? Then we can tackle each one sequentially.

AreWeDreaming commented 9 months ago

This is the PR that needs to be merged next.

AreWeDreaming commented 9 months ago

I removed all mentions of electrons.density_thermal and replaced them with just electrons.density. This should let the regression test pass. The usage of electrons.density_thermal seems weird in conjunction with just electrons.temperature. I hoped this would fix regression but it does not...

torrinba commented 9 months ago

I don't see any reviewers assigned. Is this still waiting on someone before the merge?

AreWeDreaming commented 9 months ago

Congratulations @bechtt you have fallen to one of my classic blunders.

AreWeDreaming commented 9 months ago

Need to create OMFIT PR with density_thermal change to match this PR.

torrinba commented 9 months ago

It sounds like the plan is to hold off on the electron density vs density_thermal change until we hear some clarification from IMAS devs about the possible use cases for this (through JIRA)

jmcclena commented 9 months ago

@AreWeDreaming I would think that density_thermal is the proper entry to be filled for the sample (or both?). Perhaps the failing regression could be fixed with a ods.physics_core_profiles_consistent()?

torrinba commented 9 months ago

@jmcclena do you understand why density_thermal would be useful for electrons? It seems like it only makes sense for ions

jmcclena commented 9 months ago

While not as well understood as fast ions, I believe suprathermal instabilities like fishbones have been observed with ECH heating (https://iopscience.iop.org/article/10.1088/0029-5515/47/11/022/pdf). You could also have a fast runaway electron population near a disruption.

Additionally, while there is no fast electron temperature, there is a fast parallel and perp pressure (just like how the the ions are set up)

AreWeDreaming commented 9 months ago

@jmcclena I did my thesis on non-thermal electron distributions functions generated by ECH. You have to try very hard (core density 1.e19 + 0.8 MW ECH) to generate non-thermal electron distributions, and even then they make up less than 1 % of the electron density. However, the real issue is that I cannot think of a situation where a two-fluid description of the electron distribution would be a good choice, maybe with the exception of run-away electrons, but even there you will have a hard time fitting that kind of distribution with two Maxwellians. The other thing is that all our means to measure or model n_e are using the assumption that the electrons are thermally distributed. Splitting the electron density into thermal and fast implies that this distinction has been made and something has been done to determine that the electrons are thermally distributed. I'll create a JIRA tracker at ITER and ask them for guidance. Let's see what they think.

torrinba commented 9 months ago

Shouldn't fast (and total) electron density also be set according to ITER's recommendation? https://jira.iter.org/browse/IMAS-5050

AreWeDreaming commented 9 months ago

Probably but that is out of scope for this PR

torrinba commented 9 months ago

Probably but that is out of scope for this PR

How so? It looks like you are adding the mappings to omas from MDS+ for electron density. What would be a more appropriate time to set up the proper mapping?

AreWeDreaming commented 9 months ago

Well to do it properly every time electron density_thermal is set we would need to set density_fast as well. I could do that but we have 4 more PRs to work through.

torrinba commented 8 months ago

I tried my best to test this (and the original branch) with omas viewer, but couldn't get any plots to work. That probably isn't an ideal test here (since the development has stalled), but I don't have anything better.

It could be worth running the OMFIT regression tests with this omas branch to ensure nothing breaks there. If that works I'd be comfortable with merging this

github-actions[bot] commented 6 months ago

Stale pull request message

torrinba commented 6 months ago

This passed 73 OMFIT regression tests here https://github.com/gafusion/OMFIT-source/pull/6967 and all omas tests so I don't see any reason not to merge