gafusion / omas

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

added a summary_global_quantities method #101

Closed TimSlendebroek closed 4 years ago

TimSlendebroek commented 4 years ago

Have performed some tests of the h98_tau_e_scaling:

a DIII-D case in h98: tau_exp = 0.07, tau_scaling = 0.065

However, saw a big difference in a newer DIII-D case: tau_exp_SCOPE = 0.116 tau_mhd_calc = 0.117 tau_scaling = 0.069

Also not sure what you think about the if statements, wonder how I could make it more general (on any ODS, I think there is a problem with trying to find P_aux from core_sources for example).

codecov[bot] commented 4 years ago

Codecov Report

Merging #101 into master will decrease coverage by 1.46%. The diff coverage is 39.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #101      +/-   ##
==========================================
- Coverage   79.94%   78.47%   -1.47%     
==========================================
  Files          45       45              
  Lines        7882     8089     +207     
==========================================
+ Hits         6301     6348      +47     
- Misses       1581     1741     +160     
Impacted Files Coverage Δ
omas/examples/connect_gkdb.py 83.33% <ø> (+4.76%) :arrow_up:
omas/omas_structure.py 3.00% <0.00%> (+0.02%) :arrow_up:
omas/omas_uda.py 13.27% <0.00%> (ø)
omas/omas_plot.py 68.82% <3.29%> (-4.50%) :arrow_down:
omas/omas_physics.py 71.73% <33.33%> (-6.00%) :arrow_down:
omas/omas_imas.py 23.07% <54.54%> (ø)
omas/omas_setup.py 86.11% <75.00%> (-0.41%) :arrow_down:
omas/omas_utils.py 81.44% <86.95%> (+0.59%) :arrow_up:
omas/omas_core.py 85.82% <100.00%> (ø)
omas/omas_h5.py 87.32% <100.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5dad4e1...e702907. Read the comment docs.

TimSlendebroek commented 4 years ago

I guess more testing is needed (codecov) but before doing that is there any merit in this addition / how to expand on the idea?

orso82 commented 4 years ago

@TimSlendebroek this is a great start. In general the IMAS dd should be pretty much code agnostic, so I wonder why do you think that this would only work for TGYRO

TimSlendebroek commented 4 years ago

@TimSlendebroek this is a great start. In general the IMAS dd should be pretty much code agnostic, so I wonder why do you think that this would only work for TGYRO

Well I am with you that it should be code agnostic, but my concern is that it is very likely that many codes are not using the indentifier.index. in: image

So it would crash for other codes., currently.

orso82 commented 4 years ago

Well, if that's your concern, then you should perhaps add a warning when indentifier.index is not set, but the way things are coded right now, they would not work even if someone had set indentifier.index correctly.

TimSlendebroek commented 4 years ago

@orso82 plot method added I am not sure if you like the naming, also how to add it to right_mouse on ods (if core_transport not in ods it should be greyed out)?

orso82 commented 4 years ago

@TimSlendebroek there a few issues:

  1. aspect should not be defined with respect to the radius at which the vacuum magnetic field is defined. Use the R=(ods['equilibrium.time_slice.:.profiles_1d.r_outboard'][:,-1] + ods['equilibrium.time_slice.:.profiles_1d.r_inboard'][:,-1]) / 2

  2. The whole function is not structured in a smart way. For example, why do I need to have the core_sources defined to get the summary greenwald_fraction? We should probably modularize the calculation of each quantity, and then we can have a master summary_global_quantities that runs all of the smallel global quantities functions.

  3. Please take advantage of the OMAS syntax for getting data across arrays of structures where possible. For example:

    a = (ods['equilibrium.time_slice.:.profiles_1d.r_outboard'][:,-1] - ods['equilibrium.time_slice.:.profiles_1d.r_inboard'][:,-1]) / 2
    R = (ods['equilibrium.time_slice.:.profiles_1d.r_outboard'][:,-1] - ods['equilibrium.time_slice.:.profiles_1d.r_inboard'][:,-1]) / 2
    ip = ods['equilibrium.time_slice.:.global_quantities.ip']
  4. The way things are coded right now, assume that the equilibrium, core_profiles, core_sources ODSs all have the same times, which may not bee the case. We should use the with omas_environment statement to make sure that the time bases are aligned.

  5. We need to add core_sources to the ods_sample(), so that we can easily add a regression test for this

  6. We may want to create an OMAS physics function that calculates sources totals based on the indexing scheme of core_sources.source[:].identifier.index

TimSlendebroek commented 4 years ago

@TimSlendebroek there a few issues:

1. `aspect` should not be defined with respect to the radius at which the vacuum magnetic field is defined. Use the `R=(ods['equilibrium.time_slice.:.profiles_1d.r_outboard'][:,-1] + ods['equilibrium.time_slice.:.profiles_1d.r_inboard'][:,-1]) / 2`

2. The whole function is not structured in a smart way. For example, why do I need to have the `core_sources` defined to get the summary `greenwald_fraction`? We should probably modularize the calculation of each quantity, and then we can have a master `summary_global_quantities` that runs all of the smallel global quantities functions.

3. Please take advantage of the OMAS syntax for getting data across arrays of structures where possible. For example:
    a = (ods['equilibrium.time_slice.:.profiles_1d.r_outboard'][:,-1] - ods['equilibrium.time_slice.:.profiles_1d.r_inboard'][:,-1]) / 2
    R = (ods['equilibrium.time_slice.:.profiles_1d.r_outboard'][:,-1] - ods['equilibrium.time_slice.:.profiles_1d.r_inboard'][:,-1]) / 2
    ip = ods['equilibrium.time_slice.:.global_quantities.ip']
1. The way things are coded right now, assume that the `equilibrium`, `core_profiles`, `core_sources` ODSs all have the same times, which may not bee the case. We should use the `with omas_environment` statement to make sure that the time bases are aligned.

2. We need to add `core_sources` to the `ods_sample()`, so that we can easily add a regression test for this

3. We may want to create an OMAS physics function that calculates sources totals based on the indexing scheme of `core_sources.source[:].identifier.index`

All these suggestions make a lot of sense, for 6. I am wondering it is possible to find common ground. For example, I know that the core_sources coming from ONETWO, CHEF, and TGYRO are very different. Do you think it should be the goal to make the output of these modules universal (in as far as this is possible)?

orso82 commented 4 years ago

All these suggestions make a lot of sense, for 6. I am wondering it is possible to find common ground. For example, I know that the core_sources coming from ONETWO, CHEF, and TGYRO are very different. Do you think it should be the goal to make the output of these modules universal (in as far as this is possible)?

Yes. And you have immediately identified the toughest problem with IMAS, that is different codes will fill out different fields in the DD. Which really hinders the whole idea of being able to plug-&-play different physics codes. This is even hard for us to do, even if we have full control over ONETWO, CHEF, and TGYRO! That said, we should do our best to fill out things uniformly and consistently for our, and rely on OMAS automations as much as we can to 1) avoid repetition of code and 2) have a shot at interfacing successfully with the output of codes that we do not manage.