refactor: Make model features configurable via FeatureManager#176
Open
glitch401 wants to merge 2 commits intoopenclimatefix:mainfrom
Open
refactor: Make model features configurable via FeatureManager#176glitch401 wants to merge 2 commits intoopenclimatefix:mainfrom
glitch401 wants to merge 2 commits intoopenclimatefix:mainfrom
Conversation
jacobbieker
reviewed
Oct 15, 2025
Member
jacobbieker
left a comment
There was a problem hiding this comment.
Jsut a note on this one, I don't think the model features needs to be refactored like this. Its not hardcoded currently, and I think this adds more complexity than it helps personally. The current values can be overwritten with a config dictionary, so already doing what this is trying to do.
Contributor
Author
|
Your feedback makes sense. Would you suggest that the input features to dynamically adapt according to the input features? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Description
This PR addresses a core architectural limitation by refactoring the
GraphWeatherForecasterto support configurable input and output features, removing the previously hardcoded dimensions. This change significantly improves the repository's flexibility and makes it easier to experiment with new models and feature sets.This was done by:
FeatureManagerclass (graph_weather/models/features.py) to serve as a single source of truth for feature definitions.graph_weather/configs/directory.GraphWeatherForecasterand itsConfigto accept dynamicinput_featuresandoutput_featuresarguments, decoupling the model architecture from the data schema.This work provides a crucial foundation for implementing new models (like CaFA in #105) that may use different input variables. It also aligns with the goals of issue #126 by introducing a more structured configuration approach.
To ensure full backward compatibility, a
features_default.yamlfile is included that perfectly replicates the original hardcoded 78+24 feature set.Fixes #
How Has This Been Tested?
The refactored
GraphWeatherForecasterandFeatureManagerwere tested using test scripts and interactively in a Jupyter notebook to validate:Correct parsing of the default 102-feature configuration.
Correct parsing of a minimal, custom 1-feature configuration.
Successful model instantiation and forward passes with both fake data and a real single-level ERA5 dataset, proving the end-to-end flexibility of the new system.
Yes
If your changes affect data processing, have you plotted any changes? i.e. have you done a quick sanity check?
(N/A - This change refactors the model's interface but does not alter the data processing logic itself.)
Checklist: