Skip to content

Conversation

@benbovy
Copy link
Member

@benbovy benbovy commented Jun 12, 2025

This PR integrates xvec with xproj (see xarray-contrib/xproj#27 for more context).

From the Xvec's user point of view there's no change, apart from:

  • xproj becoming a required dependency
  • some subtle differences in a couple of warning or error messages
  • a possible error raised instead of setting an undefined CRS (None) when Xproj finds multiple distinct CRSs in an Xarray Dataset or DataArray (this may be an edge case where raising an error may actually make more sense)

There's a few more details in the commit messages.

I added a couple of tests in test_xproj_integration.py, which illustrate how xvec and xproj may be used together. I can also add those examples in the documentation, although maybe it'd be safer to wait a bit until xproj becomes a bit more mature? I plan to add similar examples in xproj's documentation anyway.

benbovy added 4 commits June 12, 2025 11:46
Note: with HAS_XPROJ checks removed it is now possible that accessing
the CRS via `.proj.crs` returns an error (when multiple CRSs are found
in the Xarray object) instead of setting `None` (when HAS_XPROJ was
False). This is possible in theory but quite unlikely in practice,
though.
GeometryIndex now inherits from `xproj.ProjIndexMixin`.

Reuse functionality that has been implemented in xproj (moved from
xvec). Some warning and/or error messages may slightly change, although
I tried to keep them clear and meaningful.
Set or convert the CRS of a GeometryIndex via XProj's API.
@benbovy
Copy link
Member Author

benbovy commented Jun 12, 2025

There's some duplication in the features provided by xvec and xproj, e.g.,

  • .xvec.set_crs() vs. .xproj.map_crs(..., transform=False)
  • .xvec.to_crs() vs. .xproj.map_crs(..., transform=True)

Some of the implementation in xvec could probably be delegated to xproj, or alternatively we could deprecate the CRS-specific features in xvec in favor of xproj. It is probably better to address that later, though.

@benbovy benbovy requested a review from martinfleis June 12, 2025 13:02
Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

Thanks @benbovy, this all looks good to me.

set_crs and to_crs API is taken from geopandas and I find it a bit more intuitive than xproj's map_crs with the keyword. Though that may be simply because I am used to it. Though if Xarray's ecosystem is moving towards the latter, I am happy to deprecate stuff linked to indices from here. Although thinking about it, our CRS handling and transformation should also eventually support variable geometry, not only coordinate so might be better to keep it around.

@martinfleis
Copy link
Member

I can also add those examples in the documentation, although maybe it'd be safer to wait a bit until xproj becomes a bit more mature?

Neither xvec or xproj are mature, so I am fine with either way.

@benbovy
Copy link
Member Author

benbovy commented Jun 12, 2025

set_crs and to_crs API is taken from geopandas and I find it a bit more intuitive than xproj's map_crs with the keyword. Though that may be simply because I am used to it. Though if Xarray's ecosystem is moving towards the latter, I am happy to deprecate stuff linked to indices from here. Although thinking about it, our CRS handling and transformation should also eventually support variable geometry, not only coordinate so might be better to keep it around.

I fully agree, it is better to keep xvec's CRS API as-is, at least for now.

@martinfleis martinfleis merged commit 6c897b0 into xarray-contrib:main Jun 12, 2025
9 checks passed
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.

2 participants