Skip to content

Add edge count consistency check to graph creation tests#301

Open
osten-antonio wants to merge 8 commits into
mllam:mainfrom
osten-antonio:main
Open

Add edge count consistency check to graph creation tests#301
osten-antonio wants to merge 8 commits into
mllam:mainfrom
osten-antonio:main

Conversation

@osten-antonio

@osten-antonio osten-antonio commented Mar 1, 2026

Copy link
Copy Markdown

Describe your changes

Implements the TODO in test_graph_creation that noted edge count consistency between paired graph files was not being checked.

Each edge set in the generated graph consists of two files:

  • *_edge_index.pt — shape [2, E], connectivity between nodes
  • *_features.pt — shape [E, 3], attributes for each edge

The previous test validated each file's feature dimension independently but never verified that both files described the same number of edges E. A mismatch would pass all existing assertions silently, but cause wrong message passing or an index-out-of-bounds error at training time.

Changes:

  • Added edge_index_feature_pairs list declaring which index/feature files must match (including mesh_up/mesh_down for hierarchical graphs)
  • Collected edge counts from both files during the existing shape-check loop
  • Added a post-loop assertion over all pairs with a descriptive failure message
  • Removed the # TODO comment as it is now implemented

No new dependencies required.

Type of change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📖 Documentation (Addition or improvements to documentation)

(This is a test improvement - none of the above categories apply exactly)

Checklist before requesting a review

  • My branch is up-to-date with the target branch - if not update your fork with the changes from the target branch (use pull with --rebase option if possible).
  • I have performed a self-review of my code
  • For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values
  • I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code
  • I have updated the README to cover introduced code changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have given the PR a name that clearly describes the change, written in imperative form (context).
  • I have requested a reviewer and an assignee (assignee is responsible for merging). This applies only if you have write access to the repo, otherwise feel free to tag a maintainer to add a reviewer and assignee.

Checklist for reviewers

Each PR comes with its own improvements and flaws. The reviewer should check the following:

  • the code is readable
  • the code is well tested
  • the code is documented (including return types and parameters)
  • the code is easy to maintain

Author checklist after completed review

  • I have added a line to the CHANGELOG describing this change, in a section
    reflecting type of change (add section where missing):
    • added: when you have added new functionality
    • changed: when default behaviour of the code has been changed
    • fixes: when your contribution fixes a bug
    • maintenance: when your contribution is relates to repo maintenance, e.g. CI/CD or documentation

Checklist for assignee

  • PR is up to date with the base branch
  • the tests pass
  • (if the PR is not just maintenance/bugfix) the PR is assigned to the next milestone. If it is not, propose it for a future milestone.
  • author has added an entry to the changelog (and designated the change as added, changed, fixed or maintenance)
  • Once the PR is ready to be merged, squash commits and merge the PR.

@sadamov sadamov added the enhancement New feature or request label Mar 1, 2026
@joeloskarsson

Copy link
Copy Markdown
Collaborator

It looks like we have duplicate fixes for this in #301 and #396 now. Please discuss in #380 how to best solve this.

@sadamov sadamov added the duplicate This issue or pull request already exists label Mar 15, 2026
@joeloskarsson joeloskarsson linked an issue Mar 22, 2026 that may be closed by this pull request
@joeloskarsson

Copy link
Copy Markdown
Collaborator

This test looks good to me. Have you tested it manually (hacking the test so that it would fail), so that the error message looks good?

Could you also add a changelog entry for this?

@joeloskarsson joeloskarsson self-requested a review March 22, 2026 18:28
@joeloskarsson joeloskarsson self-assigned this Mar 22, 2026
@joeloskarsson joeloskarsson removed the duplicate This issue or pull request already exists label Mar 22, 2026
@osten-antonio

Copy link
Copy Markdown
Author

This test looks good to me. Have you tested it manually (hacking the test so that it would fail), so that the error message looks good?

@joeloskarsson Yep, I have and it works
image

@joeloskarsson joeloskarsson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sorry for taking so long to look at this again😅 The tag got lost with so many other notifications.

I guess there is a question here if we need to work on the testing for graph creation in neural-lam, as the main effort atm is to move this to using weather-model-graphs (e.g. #323 #596 ). If you are still interested in work on this, and resolving the current conflicts, I would not mind merging this in, but let me know if this still feels like time well spent with this in mind.

@osten-antonio

Copy link
Copy Markdown
Author

Sorry for taking so long to look at this again😅 The tag got lost with so many other notifications.

I guess there is a question here if we need to work on the testing for graph creation in neural-lam, as the main effort atm is to move this to using weather-model-graphs (e.g. #323 #596 ). If you are still interested in work on this, and resolving the current conflicts, I would not mind merging this in, but let me know if this still feels like time well spent with this in mind.

No worries! Yes, I'm still interested to work on this, and will be resolving the conflicts. I have checked weather-model-graphs, and it does use the same file contract iwth *_features and *_edge_index (from save.py), so I think this test should still be relevant.

@osten-antonio

Copy link
Copy Markdown
Author

@joeloskarsson I have resolved all of the conflicts, and also retested, works as expected.
image

When hacking the test:
image

@joeloskarsson joeloskarsson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

While WMG still uses the same graph format, unless I misunderstand this test is specifically testing the graphs created using create_graph.py, thereby my confusion. But nonetheless, I see no problem with this contribution, and it does improve a test that is in the codebase.

@joeloskarsson joeloskarsson added the ready Review complete - proposed for milestone label Jun 11, 2026
@joeloskarsson joeloskarsson added this to the v0.7.0 (proposed) milestone Jun 11, 2026
@joeloskarsson

joeloskarsson commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

Linting seems required though to make CI green

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

Labels

enhancement New feature or request ready Review complete - proposed for milestone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add edge-count consistency checks in `test_graph_creation

3 participants