Skip to content

Ocean/develop: add CF-compliant _FillValue attribute #533

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Ocean/develop: add CF-compliant _FillValue attribute #533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e307647
test push
wenshanw Mar 29, 2020
1f1d752
Modify Registry parser to add _FillValue attribute
wenshanw Apr 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/core_ocean/Registry.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2031,6 +2031,7 @@
<var_struct name="state" time_levs="2">
<var name="normalVelocity" type="real" dimensions="nVertLevels nEdges Time" units="m s^{-1}"
description="horizonal velocity, normal component to an edge"
missing_value="FILLVAL"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@wenshanw, this spacing needs to be done with tabs rather than spaces to be compatible with the rest of the XML in MPAS. (I know it's hard to see the difference in a lot of editors.)

/>
<var name="layerThickness" type="real" dimensions="nVertLevels nCells Time" units="m"
description="layer thickness"
Expand Down
2 changes: 1 addition & 1 deletion src/core_ocean/tracer_groups/Registry_activeTracers.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@

<var_struct name="state" time_levs="2">
<var_struct name="tracers" time_levs="2">
<var_array name="activeTracers" dimensions="nVertLevels nCells Time" type="real" packages="activeTracersPKG" >
<var_array name="activeTracers" dimensions="nVertLevels nCells Time" type="real" packages="activeTracersPKG" missing_value="FILLVAL">
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we need to add missing_value="FILLVAL" to every var_array and var(not within a var_array) in the registry? It would be nice to set missing_value="FILLVAL" as the default so no changes need to be made to the Registry.

@mgduda would a default setting of the missing_value="FILLVAL" cause problems for the atmospheric core?

An alternative is to have a global variable or attribute at the top of the registry file so each core can specify the missing_value for all variables with a single line.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@czender seemed to think that we would want to manually mark variables with _FillValue and that the default should be that variable don't have a fill value. It isn't good practice to mark variable that are valid everywhere as having a _FillValue, for example. But I'll let @czender further clarify.

I don't know what missing_value is for so I won't comment on that until I get clarity from @mgduda.

Copy link

Choose a reason for hiding this comment

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

That is correct. Variables that are valid everywhere (e.g., most of EAM) will never use a _FillValue and therefore should not a the _FillValue attribute. The only reason why is efficiency: _FillValue slows-down code. In-line code and generic analysis tools must check every single value of a variable with _FillValue before operating on that value. Lots of if's. When variables do not have _FillValue we can assume all values are valid then the analysis of the variable needs no if's. That said, it is perfect legal to have a _FillValue attribute that is never used. It is just more efficient not to.

<var name="temperature" array_group="activeGRP" units="degrees Celsius"
description="potential temperature"
/>
Expand Down
17 changes: 7 additions & 10 deletions src/tools/registry/gen_inc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1386,10 +1386,10 @@ int parse_var_array(FILE *fd, ezxml_t registry, ezxml_t superStruct, ezxml_t var
fortprintf(fd, " call mpas_add_att(%s(%d) %% attLists(const_index) %% attList, 'units', '%s')\n", pointer_name, time_lev, temp_str);
}

if ( vararrmissingval ) {
fortprintf(fd, " call mpas_add_att(%s(%d) %% attLists(const_index) %% attList, 'missing_value', %s)\n", pointer_name, time_lev, missing_value);
if ( vararrmissingval != NULL ) {
// fortprintf(fd, " call mpas_add_att(%s(%d) %% attLists(const_index) %% attList, 'missing_value', %s)\n", pointer_name, time_lev, missing_value);
// Uncomment to add _FillValue to match missing_value
// fortprintf(fd, " call mpas_add_att(%s(%d) %% attLists(const_index) %% attList, '_FillValue', %s)\n", pointer_name, time_lev, missing_value);
fortprintf(fd, " call mpas_add_att(%s(%d) %% attLists(const_index) %% attList, '_FillValue', %s)\n", pointer_name, time_lev, missing_value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a framework file, we need to merge this into the develop branch instead, and then I will merge develop into the LANL branches: ocean/develop, seaice/develop, landice/develop. @wenshanw, don't worry about that right now. This PR is convenient for testing, and I can help with the different branches after testing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I was thinking that would be the right approach, too, and that @wenshanw shouldn't have to worry about that. We need to do more testing anyway.

}
fortprintf(fd, " %s(%d) %% missingValue = %s\n", pointer_name, time_lev, missing_value);
fortprintf(fd, " %s(%d) %% constituentNames(const_index) = '%s'\n", pointer_name, time_lev, varname);
Expand Down Expand Up @@ -1431,7 +1431,6 @@ int parse_var_array(FILE *fd, ezxml_t registry, ezxml_t superStruct, ezxml_t var
return 0;
}/*}}}*/


int parse_var(FILE *fd, ezxml_t registry, ezxml_t superStruct, ezxml_t currentVar, const char * corename)/*{{{*/
{
ezxml_t struct_xml, var_xml, var_xml2;
Expand Down Expand Up @@ -1596,9 +1595,9 @@ int parse_var(FILE *fd, ezxml_t registry, ezxml_t superStruct, ezxml_t currentVa
}

if ( varmissingval != NULL ) {
fortprintf(fd, " call mpas_add_att(%s(%d) %% attLists(1) %% attList, 'missing_value', %s)\n", pointer_name, time_lev, missing_value);
// fortprintf(fd, " call mpas_add_att(%s(%d) %% attLists(1) %% attList, 'missing_value', %s)\n", pointer_name, time_lev, missing_value);
// Uncomment to add _FillValue to match missing_value
// fortprintf(fd, " call mpas_add_att(%s(%d) %% attLists(1) %% attList, '_FillValue', %s)\n", pointer_name, time_lev, missing_value);
fortprintf(fd, " call mpas_add_att(%s(%d) %% attLists(1) %% attList, '_FillValue', %s)\n", pointer_name, time_lev, missing_value);
}
fortprintf(fd, " %s(%d) %% missingValue = %s\n", pointer_name, time_lev, missing_value);

Expand Down Expand Up @@ -1672,7 +1671,7 @@ int parse_struct(FILE *fd, ezxml_t registry, ezxml_t superStruct, int subpool, c

structname = ezxml_attr(superStruct, "name");
structnameincode = ezxml_attr(superStruct, "name_in_code");

if(!structnameincode){
structnameincode = ezxml_attr(superStruct, "name");
}
Expand Down Expand Up @@ -2123,7 +2122,7 @@ int generate_immutable_streams(ezxml_t registry){/*{{{*/
fortprintf(fd, " call MPAS_stream_mgr_add_field(manager, \'%s\', \'%s\', packages=packages, ierr=ierr)\n", optname, optvarname);
else
fortprintf(fd, " call MPAS_stream_mgr_add_field(manager, \'%s\', \'%s\', ierr=ierr)\n", optname, optvarname);

}

/* Loop over arrays of fields listed within the stream */
Expand Down Expand Up @@ -2571,5 +2570,3 @@ int parse_structs_from_registry(ezxml_t registry)/*{{{*/

return 0;
}/*}}}*/