-
Notifications
You must be signed in to change notification settings - Fork 22
feat(graphs): build nodes from xarray compatible data #330
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
base: main
Are you sure you want to change the base?
feat(graphs): build nodes from xarray compatible data #330
Conversation
Jacob Mathias Schreiner seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
@JPXKQX I cannot tag you as a reviewer :) |
graphs/pyproject.toml
Outdated
@@ -55,6 +55,7 @@ dependencies = [ | |||
"torch-geometric>=2.3.1", | |||
"trimesh>=4.1", | |||
"typeguard>=4", | |||
"xarray<=2025.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be an optional dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 why xarray has become a doc
-dependency now?
Mat, why can't it be a regular dependency of graphs? As far as I know, xarray
is required by earthkit-data
, which is required by anemoi-transform
, which is required by anemoi-datasets
, which is required by anemoi-graphs
, so you would end-up with xarray anyway. Perhaps it would be just a matter of the particular version..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, I didn't see it was dependencies for docs.
However, if you decide that it should be optional, where should I put it in the pyproject file?
Something like
optional-dependencies.data = "[xarray<=2025.4]"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…com:matschreiner/anemoi-core into feat/build-nodes-from-xarray-compliant-data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Mathias, thanks for your contribution to anemoi-graphs. Great work! I've left some minor comments. The only remaining part is what to do with the xarray
dependency. Another option I see is a lazy import inside the XArrayNodes
. Something like:
try:
import xarray as xr
except ImportError:
raise ImportError("
The xarray package is required to use {self.__class__.__name__}. You can install it with 'pip install xarray'."
)
What do you think? I think we should do the same for other libraries in future (h3, ...).
7e22fd2
to
71ef05e
Compare
a5812aa
to
44e0a1f
Compare
8575c66
to
aa54902
Compare
09d212a
to
aa727cc
Compare
06fd7f8
to
a35da52
Compare
for more information, see https://pre-commit.ci
@JPXKQX I think it's a good idea with the lazy import for optional dependencies. |
This is great, thank you! The only thing I missed in the previous review was the changelog. It shouldn't be modified. When we release a new version, the CI will automatically update the changelog with the correct commits and links. |
Description
This PR adds an XArray nodebuilder for building nodes from files compatible with xarray.
Example usage:
Builds a set of nodes on the latitudes and longitudes specified in the file. Therefore this builders requires latitudes and longitudes to be defined on the file. The names of these can be specified with the
lat/lon_key
kwargs, defaulting tolat
andlon
, such that xarray can access them with its__getitem__
method on the instantiated<xarrray.Dataset>
.Additionally, since this work is related to zarr, some mock objects (that were anemoi-dataset rather than zarr mocks) called for example
mock_zarr_dataset
, have been renamed tomock_anemoi_dataset
, such that themock_zarr_dataset
is made available for actual zarr dataset mocks in the test suite.Type of Change
Issue Number
Code Compatibility
Code Performance and Testing
Dependencies
Documentation
a
Additional Notes
📚 Documentation preview 📚: https://anemoi-training--330.org.readthedocs.build/en/330/
📚 Documentation preview 📚: https://anemoi-graphs--330.org.readthedocs.build/en/330/
📚 Documentation preview 📚: https://anemoi-models--330.org.readthedocs.build/en/330/