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] Stretched graphs #51

Merged
merged 185 commits into from
Oct 21, 2024
Merged

[feature] Stretched graphs #51

merged 185 commits into from
Oct 21, 2024

Conversation

JPXKQX
Copy link
Member

@JPXKQX JPXKQX commented Sep 19, 2024

This PR implements the functionality to build stretched graphs only in an area of interest (AOI).

This includes:

  • The aoi_mask_builder objects is renamed to area_mask_builder.
  • A new node builder, StretchedTriNodes, which allows to create a stretched graph.
  • Expanding the MultiScaleEdges functionalities to support multi-scale connections in stretched graphs.

Checklist before requesting a review

  • I have performed a self-review of my code
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the documentation and docstrings to reflect the changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have ensured that the code is still pip-installable after the changes and runs
  • I have ran this on single GPU
  • I have ran this on multi-GPU or multi-node

Issue ticket number and link

Issues #8

This work would not have been possible without @theissenhelen and @JesperDramsch and the rest of the AIFS team! Thanks also to Magnus, Håvard and the MetNorway team for their contributions to this PR.


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

@JPXKQX JPXKQX self-assigned this Oct 3, 2024
@JPXKQX JPXKQX linked an issue Oct 3, 2024 that may be closed by this pull request
@JPXKQX JPXKQX added this to the Limited Area Graphs milestone Oct 3, 2024
@JPXKQX JPXKQX marked this pull request as ready for review October 3, 2024 13:29
@havardhhaugen
Copy link

Overall this looks really good, and we're pretty much able to reproduce the stretched grid graphs from the old graphgen in aifs-mono. I do have two suggestions:

  • Add a keyword argument min_distance_km to CutOutZarrDatasetNodes so this can be set in the config (Or add functionality for passing any kwargs to CutOutZarrDatasetNodes through the config).
  • Change how edge_length.invert works:
  1. Currently the edge length normalization looks weird when used with invert; in the encoder / decoder I get edgelengths between [0.96,1] and in the processor some of them have negative edge weight. I'm guessing this happens because edgelengths are inverted before the call to super().post_process(values) in EdgeLenght.post_process().
  2. Change the default value of invert to be True, giving shorter edges higher weight by default.

@JPXKQX
Copy link
Member Author

JPXKQX commented Oct 17, 2024

Thanks for taking the time to review the PR.

  • The CutOutZarrDatasetNodes will be refactored to receive a dict-like argument dataset directly from the configuration file. This change will be merged in feat: support area masking of boundary forcing #52 .
  • We left invert=False as the default because we didn't see any difference (in performance). Regarding the difference between encoder/decoder and processor edge lengths, do you use the same norm in both cases? Would you expect to invert after normalisation?

@havardhhaugen
Copy link

I used unit-max normalization for both enc/dec and proc. The way invert is implemented (length = 1 - length), I would expect this to be done after the edge lengths are normalized to [0,1].

@JPXKQX
Copy link
Member Author

JPXKQX commented Oct 18, 2024

The issue should now be resolved. Invert is applied after normalization, and a new method called "unit-range" has been introduced to map values to the [0, 1] interval, as "unit-max" only scales by dividing by the maximum.

@havardhhaugen
Copy link

Great! The edge lengths with invert and unit max now look like what I would expect.

Copy link
Member

@mchantry mchantry left a comment

Choose a reason for hiding this comment

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

LGTM, thanks very much.

@JPXKQX JPXKQX merged commit aecb1ca into develop Oct 21, 2024
109 checks passed
@JPXKQX JPXKQX deleted the feature/8-stretched-grid-graphs branch October 21, 2024 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stretched Grid graphs
4 participants