roocs / clisops

Climate Simulation Operations
https://clisops.readthedocs.io/en/latest/
Other
21 stars 9 forks source link

Regridding functionalities (powered by xESMF) #243

Closed sol1105 closed 11 months ago

sol1105 commented 2 years ago

Pull Request Checklist:

Future planned PR(s) specific for remapping:

Future planned PR(s) generally for clisops:

Zeitsperre commented 2 years ago

Hey @sol1105, I'm wondering what the state of this PR is? Do you need reviewers?

sol1105 commented 2 years ago

Hey @sol1105, I'm wondering what the state of this PR is? Do you need reviewers?

Hey @Zeitsperre, I'm sorry I wasn't able to continue to work on it in the past weeks, I try to get it into a reviewable state soon. Thanks for keeping it up to date with the master branch all this time :-)

sol1105 commented 1 year ago

@Zeitsperre @cehbrecht @agstephens I did all the changes I wanted to make to the PR. It would be great, if you gave me some feedback. It is not urgent, however. In the background I will fix the tests that are no longer running through.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4629370587

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
clisops/ops/base_operation.py 23 30 76.67%
clisops/utils/common.py 14 22 63.64%
clisops/utils/output_utils.py 24 36 66.67%
clisops/utils/dataset_utils.py 131 163 80.37%
clisops/ops/regrid.py 20 57 35.09%
clisops/core/regrid.py 388 642 60.44%
<!-- Total: 602 952 63.24% -->
Totals Coverage Status
Change from base Build 4619488998: 70.7%
Covered Lines: 1222
Relevant Lines: 1729

💛 - Coveralls
Zeitsperre commented 1 year ago

@sol1105

I wanted to know if you're still interested in finalizing the regridding implementation? I've adapted your PR to the newest package layout and will be seeing what I can do about roocs-grid (i.e. PyPI). Please let me know!

sol1105 commented 1 year ago

@Zeitsperre @huard Thank you for all the help and feedback so far. I know there is still quite a lot of work to be done with regards to the remapping functionalities, but despite that I would like to merge this PR, so there can soon be a clisops release with regridding capabilities. From there I would keep on working on the regridding functionalities (incl. working on your above suggestions) in future and smaller PRs.

Would you be ok with this or would this conflict with your use case or plans with clisops? Is there anything crucial for me to do so a merge is possible? Problematic for your use case may be that two bugs (pydata/xarray#7794 and xarray-contrib/cf-xarray#442) prevent using the more recent xarray and cf-xarray releases. Another topic was that the roocs-grids repo (holding a set of pre-defined target grids) should be released as pypi and conda package before merging this PR.

huard commented 1 year ago

Hi Martin, works for me !

sol1105 commented 11 months ago

@Zeitsperre @cehbrecht I removed python 3.8 from the CI checks because of the cf_xarray problem you adressed in roocs_utils using the following lines in the requirements.txt:

cf-xarray>=0.3.1,<=0.8.4; python_version == '3.8'
cf-xarray>=0.3.1; python_version >= '3.9'

I am not sure how such a setting can be added in the dependencies entry of the pyproject.toml, in case python 3.8 is still required. Would it simply be:

dependencies = [
  "bottleneck>=1.3.1",
  # cf-xarray is differently named on conda-forge
  "cf-xarray>=0.8.6;python_version>='3.9'",
  "cf-xarray>=0.7.5,<=0.8.0;python_version=='3.8'",
...
Zeitsperre commented 11 months ago

@sol1105

I am not sure how such a setting can be added in the dependencies entry of the pyproject.toml, in case python 3.8 is still required. Would it simply be:

dependencies = [
  "bottleneck>=1.3.1",
  # cf-xarray is differently named on conda-forge
  "cf-xarray>=0.8.6; python_version>='3.9'",
  "cf-xarray>=0.7.5,<=0.8.0; python_version=='3.8'",
...

That's exactly what you would need to do to maintain Python3.8 support. Everything is using pip under-the-hood. The only thing I'm not certain about is that I believe cf_xarray up until v0.8.4 is compatible with Python3.8 (at least, that's what I indicated in other places).

cehbrecht commented 11 months ago

@Zeitsperre I leave the last words to you :) We would like to merge this PR and make the new clisops release with the regrid operator. I have prepared daops and rook already for the new operator and it works.

Zeitsperre commented 11 months ago

Hey all,

Working on this now. I'm going to be adding a few changes to docstrings and updating some deprecated calls. I also might split the failing upstream build into its own workflow so that it isn't constantly failing. Will give everything a final overview after that's all done.

Should have something today.

sol1105 commented 11 months ago

@Zeitsperre Thanks a lot for the review and your contribution. I addressed the outstanding issues you found

Zeitsperre commented 11 months ago

Well done. Feel free to merge whenever you're ready!