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

feature: support icon graphs #53

Merged

Conversation

fprill
Copy link
Contributor

@fprill fprill commented Sep 27, 2024

This PR introduces some data structures that define a processor mesh and an encoder/decoder graph based on the icosahedral ICON grid, read from a NetCDF grid file.


📚 Documentation preview 📚: https://anemoi-graphs--53.org.readthedocs.build/en/53/

@FussyDuck
Copy link

FussyDuck commented Sep 27, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@JesperDramsch JesperDramsch left a comment

Choose a reason for hiding this comment

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

Hi @fprill, thank you so much for the contribution. It was very nice to read.

I have some comments on variable names for easier reading of the code (especially the big processing sequence), a technicality on the formatting of docstrings and I think two bigger comments.

One is on the iverbosity key, which I believe could be replaced/avoided.
The other is on potentially factoring the edge builders as they seem very similar in kind.

Again thanks for the great readability of the code, comments and style. (Didn't know about typeguard yet, I have to read up more on that. Seems quite useful.)

I marked some comments with nit as they're more nitpicks than actual blockers. Additionally, I think some of the larger comments can definitely be seen as a discussion basis rather than a blocker, let me know what you think.

Edit: I haven't left comments on a lot of nice sections in the code, as I didn't want to clutter the review even more. But just wanted to note that I did feel like there are a lot of really nice sections and it was a great read.

src/anemoi/graphs/generate/icon_mesh.py Outdated Show resolved Hide resolved
src/anemoi/graphs/generate/icon_mesh.py Outdated Show resolved Hide resolved
src/anemoi/graphs/generate/icon_mesh.py Outdated Show resolved Hide resolved
src/anemoi/graphs/edges/builder.py Outdated Show resolved Hide resolved
src/anemoi/graphs/generate/icon_mesh.py Show resolved Hide resolved
src/anemoi/graphs/generate/icon_mesh.py Outdated Show resolved Hide resolved
src/anemoi/graphs/generate/icon_mesh.py Outdated Show resolved Hide resolved
src/anemoi/graphs/generate/icon_mesh.py Outdated Show resolved Hide resolved
src/anemoi/graphs/generate/icon_mesh.py Outdated Show resolved Hide resolved
src/anemoi/graphs/nodes/builder.py Outdated Show resolved Hide resolved
@HCookie
Copy link
Member

HCookie commented Oct 16, 2024

@JPXKQX @gmertes
Can you take a look at this PR, and assist with the refactor?

Copy link
Member

@JPXKQX JPXKQX left a comment

Choose a reason for hiding this comment

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

Hi @fprill, first of all thanks for the contributions and sorry for the late review. I leave here more general comments:

  • In the last few days we have refactored a few things that may benefit this PR. We have now split the node/builders.py into different files under nodes/builders/, I think the icon nodes could go into their own ....py file. Would it be interesting to do the same with the edges?

  • We also have a PR open for moving some optimising edge attributes builders. I would say it would be beneficial to move the edge_attrs defined in icon_mesh.py to edges/attributes.py. I think for edge direction you can rely on the already implemented and move the edge_length to a new ArcLength(). In PR Migrate to torch-cluster #54 , we have already seen huge performance improvements and in the future we plan to go further to allow parallelisation with different devices. The differences are a few seconds now, but it could play a more important role in higher resolutions.

  • Other useful improvements would be to add a test for node & edge builders to avoid breaking functionality with other changes in the future without noticing and to add a docs/graphs/node_coordinates/icon_mesh.rst page with the information you consider of interest.

Thanks again for all the work, and I am free to chat to move the PR forward.

src/anemoi/graphs/edges/__init__.py Show resolved Hide resolved
src/anemoi/graphs/generate/icon_mesh.py Outdated Show resolved Hide resolved
src/anemoi/graphs/nodes/__init__.py Outdated Show resolved Hide resolved
src/anemoi/graphs/nodes/builder.py Outdated Show resolved Hide resolved
src/anemoi/graphs/generate/icon_mesh.py Show resolved Hide resolved
@fprill fprill force-pushed the feature/support-icon-graphs branch from 3624606 to 9b19fad Compare November 6, 2024 12:41
@MeraX
Copy link
Contributor

MeraX commented Nov 11, 2024

@JPXKQX, I've implemented tests for the ICON grid. Please have a look and approve the workflow to check that they are actually green.

Edit: Harrison, please apologise your erroneous mention.

@MeraX
Copy link
Contributor

MeraX commented Nov 11, 2024

@JPXKQX could you approve again? I had to fix the change log. And could you also trigger the downstream-ci to actually execute pytest?

@MeraX
Copy link
Contributor

MeraX commented Nov 11, 2024

Perhaps my bad.
https://github.com/ecmwf/anemoi-graphs/actions/runs/11782100162 shows pytest results.
Unfortunately, @fprill was not aware that his code has to support python 3.9.

@JPXKQX do you have a python 3.9 machine at hand? Could you help us find an equivalent syntax that works with Python 3.9? You may push your fixes to our branch.

Copy link
Member

@JPXKQX JPXKQX left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@JPXKQX JPXKQX merged commit 6cfcade into ecmwf:develop Nov 13, 2024
17 checks passed
@JPXKQX JPXKQX mentioned this pull request Nov 22, 2024
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.

7 participants