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

Cherry pick omas viewer dev #256

Merged
merged 21 commits into from
Mar 28, 2024
Merged

Cherry pick omas viewer dev #256

merged 21 commits into from
Mar 28, 2024

Conversation

AreWeDreaming
Copy link
Collaborator

@AreWeDreaming AreWeDreaming commented Jul 19, 2023

This cherry picked the changes from omas_viewer_dev.

  • Allows debugging of machine mappings by fixing the implicit ring inclusion in omas_machine (only tested for d3d)
  • Adds loading core_profile data from OMFITprofiles tree
  • Some changes to omas_plot to accommodate requirements of OMAS viewer.
  • Changes electron.density_thermal to electron.density throughout OMAS. This needs to be propagated to OMFIT.
  • Several small fixes in omas_physics and omas_plot that were stopping regression from working.

Ready for review

@AreWeDreaming AreWeDreaming added the WIP Work in progress label Jul 19, 2023
@AreWeDreaming
Copy link
Collaborator Author

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 AreWeDreaming changed the title WIP: Cherry pick omas viewer dev Cherry pick omas viewer dev Jul 27, 2023
@AreWeDreaming
Copy link
Collaborator Author

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

@github-actions
Copy link

Stale pull request message

@smithsp
Copy link
Member

smithsp commented Oct 13, 2023

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 AreWeDreaming requested review from jmcclena and removed request for orso82 and jmcclena December 11, 2023 19:35
@AreWeDreaming
Copy link
Collaborator Author

This is the PR that needs to be merged next.

@AreWeDreaming AreWeDreaming removed the WIP Work in progress label Dec 12, 2023
@torrinba
Copy link
Collaborator

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

@AreWeDreaming
Copy link
Collaborator Author

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

@AreWeDreaming
Copy link
Collaborator Author

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

@torrinba
Copy link
Collaborator

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

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

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

@jmcclena
Copy link
Collaborator

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

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

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

@AreWeDreaming
Copy link
Collaborator Author

Probably but that is out of scope for this PR

@torrinba
Copy link
Collaborator

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

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.

ods['core_profiles.profiles_1d.0.electrons.density_thermal'].xarray()
ods['core_profiles.profiles_1d.0.electrons.densdensity_thermality'].xarray()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This appears to be a typo

@torrinba
Copy link
Collaborator

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

Copy link

Stale pull request message

@torrinba torrinba reopened this Mar 28, 2024
@torrinba
Copy link
Collaborator

torrinba commented Mar 28, 2024

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

@torrinba torrinba merged commit e0bd766 into master Mar 28, 2024
6 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.

4 participants