gafusion / omas

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

Add shape control overlay #85

Closed eldond closed 4 years ago

eldond commented 4 years ago

Plasma shape control targets have a place in the IMAS schema (see https://jira.iter.org/browse/IMAS-2853), so they can have a standard plot overlay like all the hardware systems.

eldond commented 4 years ago

@orso82 Why does it hate my sample times?

orso82 commented 4 years ago
ValueError: Inconsistent time definitions in dict_keys(['pulse_schedule.position_control.boundary_outline.:.r.reference.time', 'pulse_schedule.position_control.boundary_outline.:.z.reference.time', 'pulse_schedule.position_control.strike_point.:.r.reference.time', 'pulse_schedule.position_control.strike_point.:.z.reference.time', 'pulse_schedule.position_control.x_point.:.r.reference.time', 'pulse_schedule.position_control.x_point.:.z.reference.time'])

I have not looked carefully but could it be that these times should be scalars?

eldond commented 4 years ago

@orso82 The schema says pulse_schedule.position_control.boundary_outline[:].r.reference.time should be flt_1d_type

eldond commented 4 years ago

@orso82 Is it complaining that the listed times are inconsistent with each other, or inconsistent with some central time? They shouldn't be inconsistent with each other, because the sample assigns them from the same source.

eldond commented 4 years ago

@orso82 The trouble is that it's comparing the shapes of pulse_schedule.position_control.strike_point.:.z.reference.time and pulse_schedule.position_control.boundary_outline.:.z.reference.time, which are and should be (2, 10) vs. (12, 10). Time is 2D because it looked through multiple channels, and it's not handling this correctly.

eldond commented 4 years ago

@orso82 My reading of the schema suggests that timing for different position_control elements shouldn't have to be the same, anyway (otherwise why keep listing it again and again?). So this check should never be applied to the items it's complaining about.

orso82 commented 4 years ago

@eldond great investigative work 👍 So I guess that when comparing time array shapes, we should always just compare that the last dimension matches?

eldond commented 4 years ago

I get that making sure the last dimension matches might matter for making a dataarray, so I guess we'll put that check in here, but in general, I don't think IMAS requires that the last dimension match. It would be convenient not to have this requirement.

I am going to try modifying the test.

codecov[bot] commented 4 years ago

Codecov Report

Merging #85 into master will decrease coverage by 0.58%. The diff coverage is 89.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
- Coverage   78.43%   77.85%   -0.59%     
==========================================
  Files          24       24              
  Lines        7147     6763     -384     
==========================================
- Hits         5606     5265     -341     
+ Misses       1541     1498      -43
Impacted Files Coverage Δ
omas/omas_core.py 81.82% <ø> (ø) :arrow_up:
omas/tests/test_omas_physics.py 95.39% <100%> (-1.41%) :arrow_down:
omas/omas_physics.py 80.6% <84.61%> (-1.43%) :arrow_down:

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 cc59561...68807aa. Read the comment docs.

eldond commented 4 years ago

The codecov report is being silly. This is a better representation: https://codecov.io/gh/gafusion/omas/compare/master...shape_ctrl/tree/omas

eldond commented 4 years ago

@orso82 codecov is being weird about the PR diff. For the actual files changed, the diff coverage is 90-100%. I think this is ready to merge.

orso82 commented 4 years ago

Nice work 👍