Add cf-python opening 'mode' and basis of integration testing - #43
Add cf-python opening 'mode' and basis of integration testing#43sadielbartholomew wants to merge 47 commits into
Conversation
dwest77a
left a comment
dwest77a
left a comment
There was a problem hiding this comment.
@sadielbartholomew could you review these suggested changes and add any comments where you think we can discuss things? Thanks
There was a problem hiding this comment.
@sadielbartholomew could you move these aliases to a separate file in core/ i.e mappings.py or aliases.py? I'd prefer to keep any hardcoded mappings/settings in one place.
| if convention is None: | ||
| def _find_spatial_dims(ds) -> Union[list, None]: | ||
| """Determine the names of the spatial dimensions.""" | ||
| lat = next((d for d in ds.dims if d.lower() in LATLON_ALIASES["latitude"]), None) |
There was a problem hiding this comment.
Does this section allow mixed conventions in your new syntax? This section tries to apply spatial slicing from the AOI in the pystac search to the data itself. I would prefer it not to work if there are any doubts about the dimension names, which is part of the reason this is only allowed to work if the dimension naming conventions match.
| lon_names = LATLON_ALIASES["longitude"] | ||
|
|
||
| if key in lat_names: | ||
| formatted_selection_args["latitude"] = value |
There was a problem hiding this comment.
Same here as the comment above about only allowing matching naming conventions for the spatial dimension selectors.
| Determine the names of the spatial dims. | ||
| Decode pystac datetime to cf-python subspace. | ||
| """ | ||
| import cf |
There was a problem hiding this comment.
My opinion on this would be to add the cf import to the top of the module, with a try/except ImportError setting cf to None if not imported. Any mentions of 'cf' within functions will not raise an error, but you may need to add a check at the point higher upstream where the cf mode is used to raise an error if 'cf' is None at that point.
| f'Bad mode value {mode}: only "xarray" and "cf" are valid modes.' | ||
| ) | ||
|
|
||
| self.mode = mode |
There was a problem hiding this comment.
Potentially raise an error here if mode is 'cf' and cf is None?
| fl = cf.read(self) | ||
| if self._cloud_format == 'kerchunk': | ||
| # Parse kwargs | ||
| mapper_kwargs = {} |
There was a problem hiding this comment.
Could you move this kerchunk logic to a separate _open_kerchunk_cf method for neatness, and also rename the other _open type methods to either _xr or _cf as needed?
| def _prepare_dataset_cf( | ||
| self, | ||
| fl: cf.FieldList, | ||
| ) -> cf.FieldList: |
There was a problem hiding this comment.
Integrate the prepare_data parameter here which should just be used to turn off the selection component entirely. If prepare_data is False you can skip the whole rest of the function and just return fl.
|
|
||
| if y is not None and x is not None: | ||
| if intersects is not None: | ||
| if intersects["type"] == "Polygon": |
There was a problem hiding this comment.
Can replace with intersects.get('type',None) == polygon to reduce the indenting?
| to access local files only. | ||
| """ | ||
| href_local = href.replace("https://dap.ceda.ac.uk", "") | ||
| ceda_dap_prefix = 'https://dap.ceda.ac.uk' |
There was a problem hiding this comment.
Add this to the aliases.py or mapping.py file mentioned above to then be imported. Thanks for highlighting that this was hardcoded!
| if isinstance(asset_id, int): | ||
| if isinstance(asset_id,int): | ||
| asset_id = self._cloud_assets[asset_id][0] | ||
|
|
There was a problem hiding this comment.
Now that cf is actually used in DataPoint could you change the cf in this function to something like cloudformat or similar, to avoid any confusion?
2 participants
Close #33, including adding the basis of integration testing for Datapoint in order to verify that the cf-python support aligns with the xarray support.
Status
Opening as draft since I need to:
xr.Datasetandcf.FieldListoutputs are as expected; andcheck_local_onlyin the test to confirm the rightlocal_onlybehaviour, which I am in the process of understanding.Otherwise essentially ready for review.