WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

Conversation

@NickGeneva
Copy link
Collaborator

@NickGeneva NickGeneva commented Dec 4, 2025

Earth2Studio Pull Request

Description

This PR does some renaming of data source with ECMWF introduced in the refactor of the IFS one.
Namely it changes the name of all the forecast sources to have a _FX and introduces IFS and IFS_ENS data source that just get the first time step from these forecast databases.

These are important because some models like AIFS need variables that only exist in the first time-step.

This will be an API break.

Closes: #562

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • The CHANGELOG.md is up to date with these changes.
  • An issue is linked to this pull request.
  • Assess and address Greptile feedback (AI code review bot for guidance; use discretion, addressing all feedback is not required).

Dependencies

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 4, 2025

Disclaimer: This is AI-generated, please review response for accuracy

Greptile Overview

Greptile Summary

This PR introduces important refactoring of ECMWF data sources by separating initial state sources from forecast sources with explicit naming (_FX suffix for forecasts). The new IFS and IFS_ENS classes provide initial state data (timestep 0 only), while the renamed IFS_FX, IFS_ENS_FX, AIFS_FX, and AIFS_ENS_FX classes continue to provide forecast data.

Key Changes:

  • Renamed base class ECMWFOpenDataSource to _ECMWFOpenDataSource (private)
  • Added new IFS and IFS_ENS classes for initial state data (timestep 0 only)
  • Renamed forecast classes: IFSIFS_FX, IFS_ENSIFS_ENS_FX, AIFSAIFS_FX, AIFS_ENSAIFS_ENS_FX
  • All classes now have comprehensive docstrings

Critical Issue:
The new IFS class is missing the required abstract method _validate_leadtime implementation, which will cause a TypeError at instantiation.

Minor Issues:

  • Typo in CHANGELOG.md: missing _FX suffix when documenting the IFS_ENS_FX rename
  • Typo in IFS docstring: "analaysis" → "analysis"

Confidence Score: 1/5

  • This PR cannot be merged safely due to a critical bug that breaks the new IFS class
  • Score reflects a critical logical error where the new IFS class is missing a required abstract method implementation. This will cause runtime failures when users try to instantiate the class. The refactoring is well-intentioned and the documentation is thorough, but the missing method makes this PR non-functional.
  • Pay close attention to earth2studio/data/ecmwf.py - the IFS class implementation is incomplete and will fail at runtime

Important Files Changed

File Analysis

Filename Score Overview
earth2studio/data/ecmwf.py 1/5 Added new IFS and IFS_ENS initial state sources, renamed forecast sources to *_FX. Critical bug: IFS class missing required _validate_leadtime method implementation.
earth2studio/data/init.py 4/5 Updated imports to export new IFS, IFS_ENS initial state sources and renamed *_FX forecast sources.
CHANGELOG.md 3/5 Documents API changes, but contains typo: missing _FX suffix for IFS_ENS_FX in the Changed section.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

@NickGeneva
Copy link
Collaborator Author

/blossom-ci

1 similar comment
@NickGeneva
Copy link
Collaborator Author

/blossom-ci

@NickGeneva
Copy link
Collaborator Author

/blossom-ci

@NickGeneva
Copy link
Collaborator Author

Coverage:

earth2studio/data/ecmwf.py                          261     23    91%   108, 182-185, 300-302, 338, 377, 394, 666, 725, 803, 807, 866, 951, 955, 1060, 1070, 1127, 1190-1191, 1236

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.

🚀[FEA]: Convert current IFS source to IFS_FX and add IFS analysis source

1 participant