Skip to content
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.

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

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

Open
sadielbartholomew opened this issue May 22, 2024 · 5 comments · May be fixed by #296
Open

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

sadielbartholomew opened this issue May 22, 2024 · 5 comments · May be fixed by #296
Labels
code tidy/refactor netCDF read Relating to reading netCDF datasets question Further information is requested

Comments

@sadielbartholomew
Copy link
Member

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:

def read(
self,
filename,
extra=None,
default_version=None,

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

def read(
filename,
external=None,
extra=None,
verbose=None,
warnings=False,
warn_valid=False,
mask=True,
domain=False,
_implementation=_implementation,
):

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 sadielbartholomew added question Further information is requested code tidy/refactor netCDF read Relating to reading netCDF datasets labels May 22, 2024
@sadielbartholomew sadielbartholomew changed the title NetCDF reading default_version argument is not exposed to user: remove it? default_version argument to NetCDFRead.read not exposed to user May 22, 2024
@sadielbartholomew sadielbartholomew changed the title default_version argument to NetCDFRead.read not exposed to user default_version argument to NetCDFRead.read not exposed: remove? May 22, 2024
@sadielbartholomew sadielbartholomew changed the title default_version argument to NetCDFRead.read not exposed: remove? default_version arg. to NetCDFRead.read not exposed: remove? May 22, 2024
@sadielbartholomew
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

sadielbartholomew commented Jun 4, 2024

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
Copy link
Contributor

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

@sadielbartholomew
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code tidy/refactor netCDF read Relating to reading netCDF datasets question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants