Splitattrs ncload

[pp-mo]

Changes netcdf loading code to specifically load into cube.attributes.globals and cube.attributes.locals
Since the same attributes are then appearing in 'cube.attributes', this in itself (i.e. without changes to saving) should have no effect.
-- Except in some minor corner cases, where an attribute is present as both a local (variable-attribute)) + global (file-attribute).

So, I think that this is (almost) fully backwards-compatible, and does not require a control switch like a FUTURE flag (but reviewer may think differently).
( N.B. I am intending to create a FUTURE flag for the new split-attributes saving behaviour, since that will definitely change behaviour quite significantly. Covered in #4986 )

The actual changes in loading code are pretty small (f736c4b)

I have had to modify change some of the existing tests from #4960, by replacing cube.attributes == <expected-result-dictionary> with dict(cube.attributes) == <expected-result-dictionary>. In principle I could have done that first, to prove pre-existing behaviour, but I think the argument is pretty solid as it is.

( Note : replaces older version #5048 )

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (8b751c5) 89.41% compared to head (522265e) 89.41%.

Additional details and impacted files

@@                 Coverage Diff                  @@
##           FEATURE_split_attrs    #5384   +/-   ##
====================================================
  Coverage                89.41%   89.41%           
====================================================
  Files                       89       89           
  Lines                    22497    22500    +3     
  Branches                  5395     5396    +1     
====================================================
+ Hits                     20116    20119    +3     
  Misses                    1636     1636           
  Partials                   745      745           

Impacted Files	Coverage Δ
lib/iris/fileformats/_nc_load_rules/helpers.py	95.96% <100.00%> (ø)
lib/iris/fileformats/netcdf/loader.py	80.79% <100.00%> (+0.21%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[trexfeathers]

@pp-mo can you clarify for me: cf_attrs_unused() is everything that doesn't get interpreted during NetCDF load? I.e. excluding coordinates, standard_name, etcetera?

[pp-mo]

@pp-mo can you clarify for me: cf_attrs_unused() is everything that doesn't get interpreted during NetCDF load? I.e. excluding coordinates, standard_name, etcetera?

Yes, I think so.
On a per-cube basis, any attribute that was read during the load-rules execution is automatically "crossed off a list".
Which happens here, and it gets reset here.
The comment about cacheing is misleading IMO : the cacheing operation is actually the setattr(self, ... line, and not the self._cf_attrs.add(... bit.

I also think the list of used attributes should be reset before each cube is processed.
But I'm just struggling a bit to find where, so I'm still looking into that. It could even be a 'hole' in the scheme.

[pp-mo]

I also think the list of used attributes should be reset before each cube is processed.

From experiment, no it does not do that, but I don't think it is a mistake.
It is the list of attributes which are not referenced during the initial "categorisation" of variables by 'fileformats/cf.py', and is not related to what happens in rules processing. It is static after that, not reset for each cube (before running rules).

The "unused_attributes" is only called from "loader._add_unused_attributes", and there it excludes any cf-specific attributes according to a list of 'recognised names'.
"_add_unused_attributes" is called once per built data object (cubes, coords, etc) during cube loading

So this list mustn't change, because it is used repeatedly (e.g. to build a new replica dimcoord for each data cube).
I guess the point is that these "unused" attributes are only fetched by calling cf_var.cf_attrs_unused(), and not through cf_var.__getattr__, so they don't get "crossed off" the list.

[pp-mo]

So this list mustn't change, because it is used repeatedly (e.g. to build a new replica dimcoord for each data cube). I guess the point is that these "unused" attributes are only fetched by calling cf_var.cf_attrs_unused(), and not through cf_var.__getattr__, so they don't get "crossed off" the list.

FTR I'm not a great fan of this code, due to the slightly 'hidden' way it does some things!
I don't like that the CFVariable classes acts as "wrappers" to the underlying netcdf variables, and I don't think providing access to pproperties of the underlying 'real' variable via the CFVariable.__getattr__ hook is a terribly good idea -- I think it would be a whole lot clearer if all such accesses were made explicitly with cf_var.cf_data.getncattr().
In fact, the CFVariable code uses self.getncattr(),
-- which only works because self.getncattr --> self.cf_data.getncattr, via the getattr` mechanism.
I really think that's a bit too clever, and doesn't help anything much.

I also find the naming of the "cf_var" and "cf_data" arguments in this code constantly confusing -- effectively sometimes "cf" in a name means it is an object of the 'fileformats/cf' module, and sometimes it means it is a netCDF4 object !

[trexfeathers]

Thanks for investigating the unused attributes @pp-mo. I agree it's confusing!

I remain surprised that it has taken so few changes to get the loading code working, however the tests are proof that you haven't missed anything.

I don't think there's any need to cover these changes with a FUTURE flag, since they only affect edge cases and not in a catastrophic way.

[pp-mo]

Brilliant, thanks for your careful approach @trexfeathers !