-
Notifications
You must be signed in to change notification settings - Fork 16
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
Glows cdf attrs update #725
Glows cdf attrs update #725
Conversation
Merge branch 'dev' of github.com:IMAP-Science-Operations-Center/imap_processing into glows_cdf_attrs_update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in reviewing, I forgot 😅 some minor requested changes from me. Thanks for doing this!
int_fillval: &int_fillval -9223372036854775808 | ||
int_uint32: &int_uint32 4294967295 | ||
int_uint32_min_one: &int_uint32_min_one 4294967294 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than this, I would just use *int_uint32 - 1
just because it seems a little overkill when int_unit32 -1
makes a lot of sense, but that's a nitpick so feel free to ignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I totally agree with you, it is quite redundant. Is there a way I can do this? Chat gpt informed me that .yaml files don't support operations like that (which could totally be wrong of course) 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, if it's not possible then it's fine how it is
FIELDNAM: Variance of filter temperature | ||
FILLVAL: *int_uint32 | ||
FORMAT: I6 | ||
LABLAXIS: Variance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to "Tmp Variance?"
FILLVAL: *int_uint32 | ||
FORMAT: I6 | ||
LABLAXIS: Voltage | ||
VALIDMAX: 65635 # Is this a typo? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should should be the same as the rest of the UINT values. It might be worth pulling them out as a default grouping, but you already have them in each specific variable so it's up to you if that's worth doing
@@ -258,7 +260,7 @@ def generate_de_dataset( | |||
"per_second": per_second, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
per_second should be within_the_second here too
Merge branch 'dev' of github.com:IMAP-Science-Operations-Center/imap_processing into glows_cdf_attrs_update
Merge branch 'dev' of github.com:IMAP-Science-Operations-Center/imap_processing into glows_cdf_attrs_update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor change but otherwise this is looking pretty good!
|
||
direct_event_attrs: | ||
direct_events: | ||
<<: *default_attrs | ||
DEPEND_O: epoch | ||
DEPEND_1: per_second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I meant this specific line - instead of DEPEND_1: per_second
it should be DEPEND_1: within_the_second
to point to our new dimension
attrs=glows_cdf_attributes.get_variable_attributes("within_the_second"), | ||
) | ||
|
||
de = xr.DataArray( | ||
direct_events, | ||
name="direct_events", | ||
dims=["epoch", "per_second", "direct_event"], | ||
dims=["epoch", "within_the_second", "direct_event"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So since this is updated here, the DEPEND_n
values that depend on this coordinate also need to be updated to match.
Merge branch 'dev' of github.com:IMAP-Science-Operations-Center/imap_processing into glows_cdf_attrs_update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for doing this!
97a46e8
into
IMAP-Science-Operations-Center:dev
Change Summary
Overview
Changed glows l1a cdf variable attributes yaml file as requested by Marek and GLOWS team.
New Dependencies
None
New Files
None
Deleted Files
None
Updated Files
Testing
None (Already covered in test_glows_l1a.py)