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

More extract features for RDRS #182

Merged
merged 13 commits into from
Apr 6, 2023
Merged

More extract features for RDRS #182

merged 13 commits into from
Apr 6, 2023

Conversation

juliettelavoie
Copy link
Contributor

@juliettelavoie juliettelavoie commented Apr 4, 2023

Pull Request Checklist:

  • This PR addresses an already opened issue (for bug fixes / features)
  • (If applicable) Documentation has been added / updated (for bug fixes / features)
  • This PR does not seem to break the templates.
  • HISTORY.rst has been updated (with summary of main changes)
    • Link to issue (:issue:number) and pull request (:pull:number) has been added

What kind of change does this PR introduce?

  • These features are in the ESPO-G workflow but I would like to have them in xscen to clean up the workflow. They were added because of RDRS.
  • feature mask: Either pass a dataset of a mask or find the mask in the catalog (need to do that in search_data_catalog). The mask is used to add nans to the data.
  • new region method sel: Instead of cutting on lat-lon (for ERA5-Land), I want to cut in rlat-rlon (for RDRS). I want this to be only defined in the config. I know it is a bit weird to put a non-clisops method in the clisops_subset function, but I also call this function in my workflow and I need it to cut the region regardless of if it is defined in rlat-rlon or lat-lon. Would it be too much of a breaking change to change the name of the function ?

Does this PR introduce a breaking change?

not, if we don't change the name of clisops_subset

Other information:

HISTORY.rst Outdated Show resolved Hide resolved
xscen/extract.py Outdated Show resolved Hide resolved
xscen/extract.py Outdated Show resolved Hide resolved
xscen/extract.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@aulemahal aulemahal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that clisops_subset is a bit misnamed. I would support a renaming, and may be a move to xscen.spatial ?

We could lower the breakingness of this change by keeping a clisops_wrapper that emits a FutureWarning for one version.

xscen/extract.py Outdated
@@ -157,6 +169,7 @@ def extract_dataset(
xr_combine_kwargs: dict = None,
preprocess: Callable = None,
resample_methods: Optional[dict] = None,
mask: Union[bool, xr.Dataset] = False,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also accept a DataArray ?

I don't find the "bool" option very useful, but I see that #183 would complete this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure for DataArray.
I guess it is only useful for me now that I already have a mask that is in the catalogue.

@RondeauG
Copy link
Collaborator

RondeauG commented Apr 5, 2023

I just saw that clisops_subset still exists as is. I think that we should rather have:

def clisops_subset(ds, region):
     warning(blabla)
     ds_subset = xs.spatial.subset(ds, region)
     return ds_subset

So as not to duplicate the code twice and have an outdated version, especially since the new function should work perfectly well with the old arguments.

@juliettelavoie
Copy link
Contributor Author

I just saw that clisops_subset still exists as is. I think that we should rather have:

def clisops_subset(ds, region):
     warning(blabla)
     ds_subset = xs.spatial.subset(ds, region)
     return ds_subset

So as not to duplicate the code twice and have an outdated version, especially since the new function should work perfectly well with the old arguments.

done!

@juliettelavoie
Copy link
Contributor Author

In extract_dataset, we ask for the name in the definition of the region in order to update cat:domain. This field was not necessary in clisops_subset, but I made it necessary in spatial.subset.

@juliettelavoie juliettelavoie merged commit 05b1a02 into main Apr 6, 2023
@juliettelavoie juliettelavoie deleted the fix-180 branch April 6, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract Features that would be nice with RDRS
3 participants