-
Notifications
You must be signed in to change notification settings - Fork 18
Refactors templates and adds QC variable attributes #129
Conversation
Refactors template by moving template boilerplate into a loop. Also adds default attributes for a number of QC variables. Requires updated logic from glider-dac repo.
Required by ioos/glider-dac#105 |
{% for var in qartod_variables %} | ||
{# if there are QARTOD variables defined, populate them #} | ||
{% if qc_var_types['qartod'] | length > 0 %} | ||
{% for qartod_var in (qc_var_types['qartod']) %} |
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.
Dunno why I put parentheses here.
<sourceName>time_qc</sourceName> | ||
<destinationName>precise_time_qc</destinationName> | ||
<sourceName>{{req_var}}</sourceName> | ||
<destinationName>{{dest_var_remaps.get(req_var, req_var)}}</destinationName> |
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.
Perhaps I should have added a comment here. This tries to get any name remappings and defaults to the original variable name if not found. defaultdict didn't seem to cover this use case elegantly, but maybe I was missing something.
<att name="long_name">v Variable Quality Flag</att> | ||
<att name="long_name">{{dest_var_remaps.get(req_var, req_var)}} Variable Quality Flag</att> | ||
{% if '_FillValue' not in qc_var_types['gen_qc'][req_var] %} | ||
<att name='_FillValue'>-127</att> |
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.
Do you need to set a type here?
<att name='_FillValue' type='int'>-127</att>
https://coastwatch.pfeg.noaa.gov/erddap/download/setupDatasetsXml.html#addAttributes
<addAttributes> | ||
<att name="ioos_category">Quality</att> | ||
<att name="long_name">v Variable Quality Flag</att> | ||
<att name="long_name">{{dest_var_remaps.get(req_var, req_var)}} Variable Quality Flag</att> |
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.
Does the output from {{dest_var_remaps.get(req_var, req_var)}}
result in the variable name + '_qc'? If so I think we need to drop the _qc
in the long name.
E.G.
<destinationName>u_qc</destinationName>
<att name="long_name">u Variable Quality Flag</att>
<addAttributes> | ||
<att name="ioos_category">Quality</att> | ||
<att name="long_name">v Variable Quality Flag</att> | ||
<att name="long_name">{{dest_var_remaps.get(req_var, req_var).replace('_qc', '')}} Variable Quality Flag</att> |
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.
Ew. May refactor later; too much logic in the templates.
I would recommend adding both I'm also not a fan of assigning the QARTOD flag of 2 for the |
flag_values, and flag_meanings defined #} | ||
<att name="flag_values">1B, 2B, 3B, 4B, 9B</att> | ||
<att name="flag_meanings">GOOD NOT_EVALUATED SUSPECT BAD MISSING</att> | ||
<att name="_FillValue" type='byte'>2</att> |
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.
@kerfoot what's your opinion on setting the _FillValue to a 2 (NOT_EVALUATED) flag?
I agree with @kwilcox. The _FillValue should be a value not listed in variable:flag_values |
Adds flag_values, flag_meanings, _FillValue attributes to *_qc variables if they are not present in the original dataset.
{% if '_FillValue' not in qc_var_types['gen_qc'][req_var] %} | ||
<att name='_FillValue' type='byte'>-127</att> | ||
{% if req_var not in qc_var_types['gen_qc'] %} | ||
<att name="flag_values">0,1,2,3,4,5,6,7,8,9</att> |
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.
Why do the gen_qc
variables have a different syntax for flag_values
when compared to the qartod
variables?
<att name="flag_values">0,1,2,3,4,5,6,7,8,9</att>
vs
<att name="flag_values">1B, 2B, 3B, 4B, 9B</att>
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.
Fixed in latest to comply with CF.
Uses same types in flag_attributes values, per CF standard section 3.5: "The flag_masks and flag_values attributes are both the same type as the variable to which they are attached, and contains a list of values matching unique bit fields.""
@@ -310,7 +310,7 @@ | |||
<att name="ioos_category">Quality</att> | |||
<att name="long_name">{{dest_var_remaps.get(req_var, req_var).replace('_qc', '')}} Variable Quality Flag</att> | |||
{% if req_var not in qc_var_types['gen_qc'] %} | |||
<att name="flag_values">0,1,2,3,4,5,6,7,8,9</att> | |||
<att name="flag_values">0b, 1b, 2b, 3b, 4b, 5b, 6b, 7b, 8b, 9b</att> |
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 mobile and can't test this in ERDDAP or the NCML syntax at the moment... but I believe this should be:
<att name="flag_values" type="byte">0 1 2 3 4 5 6 7 8 9</att>
I don't know where the b
is coming from and spaces are used as the separator in NcML/NetCDF array attributes.
Can you add any tests for these templates? Maybe loading one into NetCDF Java with some fill data and testing the attributes?
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 agree, but I would think it needs to be a byteList, right?
<att name="flag_values" type="byteList">0 1 2 3 4 5 6 7 8 9</att>
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.
It should be byte
. byteList
isn't a thing in the NcML Schema. See: http://www.unidata.ucar.edu/software/thredds/current/netcdf-java/ncml/AnnotatedSchema4.html, the "attribute Element" section and the "DataType Type" attribute.
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.
Oh I thought this was an ERDDAP datasets.xml template so I was looking at the following:
https://coastwatch.pfeg.noaa.gov/erddap/download/setupDatasetsXml.html#addAttributes
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.
My mistake, you are right, I thought this was an NcML file! byteList
is correct.
@@ -331,7 +331,7 @@ | |||
<att name="ioos_category">Quality</att> | |||
{# all qartod variables should have _FillValue, missing_value, | |||
flag_values, and flag_meanings defined #} | |||
<att name="flag_values">1B, 2B, 3B, 4B, 9B</att> | |||
<att name="flag_values">1b, 2b, 3b, 4b, 9b</att> |
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.
Same as other comment
Refactors template by moving template boilerplate into a loop. Also
adds default attributes for a number of QC variables. Requires updated
logic from glider-dac repo.