Skip to content
This repository has been archived by the owner on Dec 20, 2024. It is now read-only.

Feature/flexible remappers #88

Merged
merged 22 commits into from
Dec 19, 2024

Conversation

OpheliaMiralles
Copy link
Contributor

Related to #82. Rename remapper.py in multimapper.py because it was created to handle the angle (1 variables) to cos/sin (2 variables) case, which is a bit more involved than the 1-to-1 remapping. Mappings are in a module mappings.py, which is aimed at being easy to modify so that everyone can add 1-to-1 mappings. Finally, 1-to-1 remapping in place is done in remapper.py. Feel free to comment/edit/commit on top if something is unclear/does not work. Names need to change in the config from __target__: anemoi.models.preprocessing.remapper.Remapper to __target__: anemoi.models.preprocessing.multimapper.Multimapper for users of the angle to cos_sin mapping.

@FussyDuck
Copy link

FussyDuck commented Nov 22, 2024

CLA assistant check
All committers have signed the CLA.

@sahahner
Copy link
Member

Hi Ophelia,
Thank you for this nice PR. I will have a look at the contributions.
I am worried about changing the name of the remapper, because this will enforce not only changes in everyone's config files (for those who are using the remapper at the moment) but will also require another PR on anemoi-training. Can we possibly find another name for the 1-to-1 remapper?

@OpheliaMiralles
Copy link
Contributor Author

Dear Sarah,
I think "remapper" actually suits better the 1-to-1 very common usecase, while the angle to cos/sin is more of an edge case (with lists of indices involved etc). In the config, it would make the target go from remapper.Remapper to multimapper.Multimapper, which is not such a big change, don't you think?

@OpheliaMiralles OpheliaMiralles self-assigned this Dec 6, 2024
@sahahner
Copy link
Member

sahahner commented Dec 11, 2024

Hi Ophelia,

I think the optimal solution would be to have one remapper-preprocessor, that can handle the many-to-many and the one-to-one case.
The name remapper is very general, so I would like to keep it for this future implementation. At the moment, I am still trying to figure out how to reduce the memory consumption of the remapper, as we noticed that we run into issues when we train a model on 9km resolution.

So, I would propose naming the old remapper multimapper and the one-to-one mapper, which this PR introduces, monomapper. Later then we can use remapper for the general solution. I hope you like this proposed solution.

Would it be possible to merge the latest version of develop into this one, please?

@OpheliaMiralles
Copy link
Contributor Author

OpheliaMiralles commented Dec 13, 2024

tests pass, ensured backward compatibility, updated multimapper, added tests. Can this please be approved/reviewed before tuesday? I leave for holidays then... And I'd like it to be merged before the refactoring of anemoi graphs/models/training.
@sahahner

core Outdated Show resolved Hide resolved
Copy link
Member

@sahahner sahahner left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you very much for incorporating all my comments.

@mchantry mchantry mentioned this pull request Dec 18, 2024
@sahahner sahahner merged commit 489a241 into ecmwf:develop Dec 19, 2024
16 checks passed
JPXKQX added a commit that referenced this pull request Dec 19, 2024
theissenhelen pushed a commit that referenced this pull request Dec 19, 2024
* Add one-to-one mapper -> Monomapper
* Remapper (one-to-many mapper) is now called Multimapper

---------

Co-authored-by: Sara Hahner <[email protected]>
JPXKQX added a commit that referenced this pull request Dec 19, 2024
* fix: move PR #88 to unreleased section

* fix: add link to PR
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants