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

Update interpolate action to output HEALPix_nested directly #50

Draft
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

tweska
Copy link
Member

@tweska tweska commented Jan 20, 2025

This change removes the need to use the renumber-healpix action to interpolate to a HEALPix_nested. You can now do it in the interpolate action directly.

Draft because I think we might want to consider adding a few more things:

  • Extra 1024 test, I think the data needed already exists so we might as well.
  • Tests in the test/action/interpolate directory which will remain after we remove renumber-healpix
  • Some sort of warning message in the renumber-healpix, alerting users of this change.

caching: false
- type: metadata-mapping
mapping: '{~}/mapping_1024.yaml'
- type: renumber-healpix
Copy link
Member

Choose a reason for hiding this comment

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

I would remove the renumbering option altogether, as well as associated code -- this is legacy and not needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing renumber HEALPix requires a lot of modifications in climateDT. RAPS, multIO plans, and moreover there is still the FESOM interpolation (interpolation from matrix) pending question (who will remap the matrices?). My understanding is that now we are in a very busy phase and it is better to not touch for the moment.

caching: false
- type: metadata-mapping
mapping: '{~}/mapping_32.yaml'
- type: renumber-healpix
Copy link
Member

Choose a reason for hiding this comment

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

Again I'd remove all cases/code refering to renumbering

pmaciel
pmaciel previously approved these changes Jan 20, 2025
Copy link
Member

@pmaciel pmaciel left a comment

Choose a reason for hiding this comment

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

Excellent -- though I'd remove teh renumebering functionality/tests

@FussyDuck
Copy link

FussyDuck commented Jan 21, 2025

CLA assistant check
All committers have signed the CLA.

@tweska
Copy link
Member Author

tweska commented Jan 21, 2025

Added a test test with reference data generated without the changes in the interpolate action. The output is exactly the same as before, even if you use the original plan. As requested by @stefaniereuter

The commit from @MircoValentiniECMWF is a tool which lets you convert the matrices needed for fesom, this is work in progress. We still need to test it and possibly make a few changes.

@MircoValentiniECMWF
Copy link
Collaborator

MircoValentiniECMWF commented Jan 21, 2025

some more points:

  1. It will be nice to add to this script "src/multio/action/interpolate/generate_fesom_caches.sh" the management of the new command line flag to reorder the matrix ( or to create two scripts generate_fesom_cache_ring.sh generate_fesom_cache_nested.sh)
  2. @dsarmany what shall we do with the HEALPix.* class? now I just copied the files in the interpolate action, but we have a copy in the renumber HEALPix action. Maybe we should put this calss in utils? or is it ok to have a duplication since renumberHELAPix should be removed in the future?
  3. We need to update the interpolate action also for the "matrix" interpolation method. Same modification we made for the other method we need to inject the ordering convention (that has to be in the matrix name.)
  4. It will be nice to have a test for the FESOM workflow. csv -> mat and then using the mat in the interpolate action to do an interpolation from FESOM grid to HEALPix nested directly

@pmaciel
Copy link
Member

pmaciel commented Jan 22, 2025

some more points:

1. It will be nice to add to this script "src/multio/action/interpolate/generate_fesom_caches.sh" the management of the new command line flag to reorder the matrix ( or to create two scripts generate_fesom_cache_ring.sh generate_fesom_cache_nested.sh)

2. @dsarmany what shall we do with the HEALPix.* class? now I just copied the files in the interpolate action, but we have a copy in the renumber HEALPix action. Maybe we should put this calss in utils? or is it ok to have a duplication since renumberHELAPix should be removed in the future?

3. We need to update the interpolate action also for the "matrix" interpolation method. Same modification we made for the other method we need to inject the ordering convention (that has to be in the matrix name.)

4. It will be nice to have a test for the FESOM workflow. csv -> mat and then using the mat in the interpolate action to do an interpolation from FESOM grid to HEALPix nested directly

For one of the points above, you can also modify the mir-codec-to-weight-matrix tool to renumber the input and/or output indices directly.

@dsarmany
Copy link
Collaborator

  1. @dsarmany what shall we do with the HEALPix.* class? now I just copied the files in the interpolate action, but we have a copy in the renumber HEALPix action. Maybe we should put this calss in utils? or is it ok to have a duplication since renumberHELAPix should be removed in the future?

I think it is okay to keep the duplicates, given that we want to get rid of the whole action anyway. I am happy to give climate DT a bit of a breathing space right now. But at some point, and still sooner rather than later, I agree with @pmaciel that we should just simply remove this action and force the climate-DT plans to be updated.

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

Successfully merging this pull request may close these issues.

5 participants