Skip to content

[FB] [PI-3474] (WIP) Ensure flags load/save without units #3613

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

Merged
pp-mo merged 7 commits into from
Jun 5, 2020
Merged

[FB] [PI-3474] (WIP) Ensure flags load/save without units #3613

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f9e1b1e
ensure flags load/save without units
stephenworsley Dec 19, 2019
6c74050
add load test
stephenworsley Dec 19, 2019
76d28bd
minor fixes
stephenworsley Dec 19, 2019
6274f2a
change method for checking cf_var attributes
stephenworsley Jun 3, 2020
1267b8f
Fix tests
stephenworsley Jun 3, 2020
f915703
change cf_var attribute retrieval from in to hasattr
stephenworsley Jun 4, 2020
5665d11
change dependencies
stephenworsley Jun 5, 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
4 changes: 4 additions & 0 deletions lib/iris/fileformats/_pyke_rules/fc_rules_cf.krb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1689,6 +1689,10 @@ fc_extras
if np.issubdtype(cf_var.dtype, np.str_):
attr_units = cf_units._NO_UNIT_STRING

if "flag_values" in cf_var._nc_attrs or "flag_masks" in cf_var._nc_attrs \

@pp-mo pp-mo Jun 2, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we should be using a private property of CFVariable here if we can avoid it.
I've just spent much too long trying to understand exactly what CFVariable does + how,
and I would say, please let's not reply on that implementation here !

I think I would use any(hasattr(cf_var, name) for name in (...)).

It's true, this will be less efficient because hasattr depends entirely on getattr
On the other hand, it will only fetch them once per variable, as the CFVariable implementation does ensure that.
Given that netcdf files don't have the proliferation of small things that field-based formats do, I think it will be ok for performance -- although we could test that ?

@stephenworsley stephenworsley Jun 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So it looks like there is some unusual behaviour associated with getting atrributes from cf_var where accessing an attribute will cause it to not be picked up as an attribute in the final object. I think this is due to this piece of code

iris/lib/iris/fileformats/cf.py

Lines 175 to 183 in 715d019

def __getattr__(self, name):
# Accessing netCDF attributes is surprisingly slow. Since
# they're often read repeatedly, caching the values makes things
# quite a bit faster.
if name in self._nc_attrs:
self._cf_attrs.add(name)
value = getattr(self.cf_data, name)
setattr(self, name, value)
return value

It seems like in the current implementation, "attributes" contains everything which has not already been accessed in this way. Trying to access these attributes prematurely causes them to be ignored later on.
I believe this is why I chose to take attributes from cf_var._nc_attrs rather than directly from cf_var. I tested your suggestion and it looks like this would cause attributes to load incorrectly.

This does mean that we are bypassing a mechanism which is specifically designed to improve performance. I have a feeling that the ideal solution may have to involve separating the mechanism for accessing cf_var attributes from the mechanism for chosing which cf_var attributes to ignore when setting iris attributes.

@pp-mo pp-mo Jun 3, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@stephenworsley I have a feeling that the ideal solution may have to involve separating the mechanism for accessing cf_var attributes from the mechanism for chosing which cf_var attributes to ignore when setting iris attributes.

You're dead right !
I do think that the overlapping schemes in CFVariable generate a whole heap of extra confusion.
( I spent quite some time trying to work out why the 'ignored' and 'unused' ideas work in just the the way that they do. )

@stephenworsley stephenworsley Jun 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Having said this, checking the attributes of cf_var.cf_data seems to work while relying on more public API.

@pp-mo pp-mo Jun 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So, I'm really sorry for making a duff suggestion !
A simple "clean" solution would be to test hasattr(cf_var.cf_data) instead of hasattr(cf_var). But, like you, I'm still a bit concerned about bypassing the speedup mechanism (even though I suspect that at least some of it is probably unnecessary).

Another really simply fix for my original objection would be to retain your existing code solution, but just make cf_var._nc_attrs public -- by renaming, removing the underscore.
How would you feel about that ?

( Note: there aren't many existing uses of this, and they are all within cf.py )

@stephenworsley stephenworsley Jun 3, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I feel like either of these could work. To be honest, I don't have a strong enough picture of how cf_var is structured to have a clear idea of which feels more correct. In either case, I suspect both approaches would require changing the a lot of the tests currently using a mocked cf_var.

or "flag_meanings" in cf_var._nc_attrs:
attr_units = cf_units._NO_UNIT_STRING

# Get any assoicated calendar for a time reference coordinate.
if cf_units.as_unit(attr_units).is_time_reference():
attr_calendar = getattr(cf_var, CF_ATTR_CALENDAR, None)
Expand Down
6 changes: 3 additions & 3 deletions lib/iris/fileformats/netcdf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1776,7 +1776,7 @@ def _inner_create_cf_cellmeasure_or_ancil_variable(
# Add the data to the CF-netCDF variable.
cf_var[:] = data

if dimensional_metadata.units != "unknown":
if dimensional_metadata.units not in ("no_unit", "unknown"):
_setncattr(cf_var, "units", str(dimensional_metadata.units))

if dimensional_metadata.standard_name is not None:
Expand Down Expand Up @@ -1942,7 +1942,7 @@ def _create_cf_coord_variable(self, cube, dimension_names, coord):
# Deal with CF-netCDF units and standard name.
standard_name, long_name, units = self._cf_coord_identity(coord)

if units != "unknown":
if units not in ("no_unit", "unknown"):
_setncattr(cf_var, "units", units)

if standard_name is not None:
Expand Down Expand Up @@ -2387,7 +2387,7 @@ def store(data, cf_var, fill_value):
if cube.long_name:
_setncattr(cf_var, "long_name", cube.long_name)

if cube.units != "unknown":
if cube.units not in ("no_unit", "unknown"):
_setncattr(cf_var, "units", str(cube.units))

# Add the CF-netCDF calendar attribute.
Expand Down
1 change: 0 additions & 1 deletion lib/iris/tests/results/netcdf/netcdf_save_no_name.cdl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ variables:
double dim1(dim1) ;
dim1:units = "m" ;
char unknown_scalar(string6) ;
unknown_scalar:units = "no_unit" ;

// global attributes:
:Conventions = "CF-1.7" ;
Expand Down
49 changes: 49 additions & 0 deletions lib/iris/tests/test_netcdf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,55 @@ def test_ancillary_variables(self):
)
self.assertEqual(avs[0], expected)

def test_status_flags(self):
# Note: using a CDL string as a test data reference, rather than a binary file.
ref_cdl = """
netcdf cm_attr {
dimensions:
axv = 3 ;
variables:
int64 qqv(axv) ;
qqv:long_name = "qq" ;
qqv:units = "1" ;
qqv:ancillary_variables = "my_av" ;
int64 axv(axv) ;
axv:units = "1" ;
axv:long_name = "x" ;
byte my_av(axv) ;
my_av:long_name = "qq status_flag" ;
my_av:flag_values = 1b, 2b ;
my_av:flag_meanings = "a b" ;
data:
axv = 11, 21, 31;
my_av = 1b, 1b, 2b;
}
"""
self.tmpdir = tempfile.mkdtemp()
cdl_path = os.path.join(self.tmpdir, "tst.cdl")
nc_path = os.path.join(self.tmpdir, "tst.nc")
# Write CDL string into a temporary CDL file.
with open(cdl_path, "w") as f_out:
f_out.write(ref_cdl)
# Use ncgen to convert this into an actual (temporary) netCDF file.
command = "ncgen -o {} {}".format(nc_path, cdl_path)
check_call(command, shell=True)
# Load with iris.fileformats.netcdf.load_cubes, and check expected content.
cubes = list(nc_load_cubes(nc_path))
self.assertEqual(len(cubes), 1)
avs = cubes[0].ancillary_variables()
self.assertEqual(len(avs), 1)
expected = AncillaryVariable(
np.ma.array([1, 1, 2], dtype=np.int8),
long_name="qq status_flag",
var_name="my_av",
units="no_unit",
attributes={
"flag_values": np.array([1, 2], dtype=np.int8),
"flag_meanings": "a b",
},
)
self.assertEqual(avs[0], expected)

def test_cell_measures(self):
# Note: using a CDL string as a test data reference, rather than a binary file.
ref_cdl = """
Expand Down