-
Notifications
You must be signed in to change notification settings - Fork 22
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
Load selected variables instead of making them virtual #69
Conversation
virtualizarr/xarray.py
Outdated
@@ -21,6 +21,7 @@ def open_virtual_dataset( | |||
filepath: str, | |||
filetype: Optional[str] = None, | |||
drop_variables: Optional[List[str]] = None, | |||
load_variables: Optional[List[str]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure "load" is the best name for this kwarg, because these variables will still be lazy. But what else is the opposite of "virtual"? "Real" also has incorrect connotations (and "inline" makes no sense here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to "loadable", which is marginally better because it doesn't imply that they have already been loaded.
|
||
|
||
|
||
def dataset_from_kerchunk_refs( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're now only using this function in internal tests
# TODO really we probably want a dedicated xarray backend that iterates over all variables only once | ||
ds = xr.open_dataset(filepath, drop_variables=drop_variables) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point it might be nice to have a dedicated function / xarray backend that only opens the legacy file once, instead of opening it once with kerchunk and once with with xarray.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
==========================================
- Coverage 90.57% 90.18% -0.40%
==========================================
Files 14 14
Lines 955 998 +43
==========================================
+ Hits 865 900 +35
- Misses 90 98 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Towards implementing case (1) of #62, at least the opening step. We would still need to be able to save these loaded variables out (to kerchunk and zarr ideally).
Mostly works but there is a subtlety with the difference between aVariable
and anIndexVariable
causing the test to fail.