Skip to content

Conversation

@JPXKQX
Copy link
Member

@JPXKQX JPXKQX commented Jul 29, 2025

Description

This PR includes some mappers that support dynamic graphs. The main difference is that now the graph is passed as an argument to the forward() method.

What problem does this change solve?

The current mappers set the graph during its creation.

What issue or task does this change relate to?

Additional notes

As a contributor to the Anemoi framework, please ensure that your changes include unit tests, updates to any affected dependencies and documentation, and have been tested in a parallel setting (i.e., with multiple GPUs). As a reviewer, you are also responsible for verifying these aspects and requesting changes if they are not adequately addressed. For guidelines about those please refer to https://anemoi.readthedocs.io/en/latest/

By opening this pull request, I affirm that all authors agree to the Contributor License Agreement.

@JPXKQX JPXKQX added enhancement New feature or request ATS Approval Needed Approval needed by ATS labels Jul 29, 2025
@github-actions github-actions bot added the bug Something isn't working label Jul 29, 2025
@ssmmnn11
Copy link
Member

I wonder if we actually need an extra dynamic mapper class.

@anaprietonem
Copy link
Contributor

I wonder if we actually need an extra dynamic mapper class.

As in Simon you mean we could directly refactor the existing mappers so all are dynamic by default? That would significantly reduce the code complexity ... so I like the idea. But it would also mea breaking/not-being to reuse old checkpoints?

@ssmmnn11
Copy link
Member

ssmmnn11 commented Sep 3, 2025

I wonder if we actually need an extra dynamic mapper class.

As in Simon you mean we could directly refactor the existing mappers so all are dynamic by default? That would significantly reduce the code complexity ... so I like the idea. But it would also mea breaking/not-being to reuse old checkpoints?

We discussed what would need to be changed. Idea is that mapper should only receive edges, nodes, edge_attr. It will mean breaking things unfortunately.

@mchantry
Copy link
Member

Refactor planned as described in #552

@mchantry mchantry added ATS Approved Approved by ATS and removed ATS Approval Needed Approval needed by ATS labels Sep 30, 2025
@mchantry
Copy link
Member

mchantry commented Oct 1, 2025

Superseeded by #574 which follows the outline illustrated in #552

@mchantry mchantry closed this Oct 1, 2025
@github-project-automation github-project-automation bot moved this from Now In Progress to Done in Anemoi-dev Oct 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ATS Approved Approved by ATS bug Something isn't working enhancement New feature or request models training

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants