search

NCAS-CMS / cfdm

A Python reference implementation of the CF data model
http://ncas-cms.github.io/cfdm
MIT License
28 stars 11 forks source link

`default_version` arg. to `NetCDFRead.read` not exposed: remove? #298

Open sadielbartholomew opened 3 months ago

sadielbartholomew commented 3 months ago

The netCDF-specific read, i.e. NetCDFRead.read method, has an argument default_version which isn't documented but according to a comment over logic using the corresponding variable, it supports a "default version provided by the user" for the Conventions:

https://github.com/NCAS-CMS/cfdm/blob/e948eef4abf65b12fe1c0f8785f185ed9a0c3cd0/cfdm/read_write/netcdf/netcdfread.py#L774-L778

however in our user-exposed read function, there is no way to access it:

https://github.com/NCAS-CMS/cfdm/blob/e948eef4abf65b12fe1c0f8785f185ed9a0c3cd0/cfdm/read_write/read.py#L11-L21

so there is no way to apply that in the intended way, as a user. It is not documented at all across the documentation either.

It seems like this is dead code which needs to be removed, but I've opened this as a question in case we instead want to revive it by exposing it through the read function. @davidhassell, what do you think is best?

If we want to remove the several lines of logic that concern it, I can do so as part of #296 which touches the same area of code.

sadielbartholomew commented 3 months ago

There is another parameter I would like to expose in cf.read that is available in cf.NetCDFRead.read, namely _scan_only. This one is of course only intended for internal use, but I can't find an elegant-enough (non-contrived) way to query on the read variables registered (read_vars dict) without it, and I want to see those for testing e.g. in the associated PR #296.

That said, I think it could be a useful user-facing method - so I propose to upgrade it to one, possibly as part of the API review for cf v4.0.0.

davidhassell commented 3 months ago

Hi Sadie - what is the use case for querying the read_vars dict? (not doubting there is one - just wondered what you had in mind)

sadielbartholomew commented 3 months ago

In #296 I want to check that the correct read_vars["file_version"] and read_vars["UGRID_version"] (etc. for CFA version) get set, though perhaps we can just test that the ultimate Conventions property gets set, if we set it to the default e.g. CF-1.11 when an invalid value such as 1.6/1.7 is set?

davidhassell commented 3 months ago

Is the answer to that to add those keys to the DEBUG logging from NetCDFRead?

sadielbartholomew commented 3 months ago

Is the answer to that to add those keys to the DEBUG logging from NetCDFRead?

I have done that already and so I guess we can indeed test by searching for the 'file_version' result in the log output string, and that would mean no need to expose the _scan_only keyword. Sounds like a plan! When I move on from ES-DOC work I'll do that for #296 and finish and open it for review. Thanks.