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

Refactoring of the grid handling to possibly support more complex data structures #33

Open
wants to merge 53 commits into
base: main
Choose a base branch
from

Conversation

oloapinivad
Copy link
Collaborator

A first tests of GridInspector() class and some gridtype based operation as discussed in #32
Target is still far but the idea is in place.

@oloapinivad
Copy link
Collaborator Author

🚀 🚀 🚀

@oloapinivad
Copy link
Collaborator Author

oloapinivad commented Oct 16, 2024

After all this machinery, everything is now working as it was originally, meaning that the Gridtype introduction allow for backward compatibility, at least with the current testing strategy

A lot of things must be still improved:

  • Rationalize the regrid2d and regrid3d in order
  • Read the vertical_dim from the weight when initialize the Gridtype through weights
  • Cleaner GridInspector dealing with Dataset or DataArray.
  • when writing data from xarray dataset, bring also bounds variable
  • Verify if the vertical_dim should be a possible argument of the Regrid class to guarantee flexibility
  • Extend tests to make it compatible with AQUA cases. There are cases within AQUA with complex cdo_extra arguments which are not covered here. Also increase coverage should be an option.
  • Review docstrings
  • Documentation: a first draft is available and set up on read the docs
  • Compatibility test with AQUA: need to run AQUA with a specific installation of smmregrid
  • Have a CdoGenerate() class which support for weights creations
  • Move the generation capabilities of areas from AQUA to smmregrid
  • Consolidate the apply_weights_function()

Long terms ideas

  • Move some the grid handling from AQUA to smmregrid
  • cdo_weights_generator can receive as an argument Gridtype and store some of it into the weights file to be later reused. This is fundamental for the future support for multiple grids

@oloapinivad
Copy link
Collaborator Author

@jhardenberg @mnurisso this can be considered ready to me. I will move the remaining issues (e.g. the creation of a class for generation of weights) to new issues and possibly develop this in the future.

It will be interesting to see which of the features of AQUA should be moved here for a cleaner operativity. For example, now all the dimensions guessing is done inside smmregrid so it should be completely redundant what is done in AQUA. Also, given the properties of the tool, also the grid are generator can be moved here as a specific class (or a twin object of the generation of weights).

@@ -21,7 +21,8 @@ jobs:
strategy:
fail-fast: false
matrix:
python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"]
#python-version: ["3.8", "3.9", "3.10", "3.11", "3.12"]
python-version: ["3.12"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Has this to be reverted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not really, it is a lot of calculation, we might want to avoid

@@ -29,7 +29,7 @@ jobs:
environment-file: environment.yml
environment-name: smmregrid
cache-downloads: true
cache-environment: true
cache-environment: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd revert this as well before merging

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well, it depends if we keep or not the python matrix. I have noticed test failure when retrieving the cached env with matrix active, not fully sure if these are related though.

@oloapinivad
Copy link
Collaborator Author

oloapinivad commented Oct 18, 2024

I spotted a couple of cases that slipped through the tests, most importantly the one when the time dimension has been dropped. We could make a bit of discussion on the implementation of the GridType class in order to see if there are better possible definitions. Now everything should be back in place.

@oloapinivad
Copy link
Collaborator Author

Trying to address #34, first tests are positive. We need to extract more information from the CDO weights, since there are data structure details on which we can build

@oloapinivad oloapinivad linked an issue Oct 21, 2024 that may be closed by this pull request
@oloapinivad
Copy link
Collaborator Author

I created a new class CdoGenerate() which has two exposed method, one is weights() and the other is areas() so that it can produce both weights and areas and easily port them to python.

I tried to divide the many options available in order that the ones CDO-related all fill into the class while the other goes into the method. Feel free to suggest improvements. I still need to write some tests to check everything works as expected, but it is quite straightforward since I am using direct cdo calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants