Skip to content
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

Add Coordinates.from_xindex method (+ refactor API doc) #10000

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

benbovy
Copy link
Member

@benbovy benbovy commented Jan 29, 2025

This PR adds a new Coordinates.from_xindex() method as convenience for creating a new Coordinates object directly from an existing xarray.Index object. This will be useful, e.g., for #9543.

I picked the name from_xindex() as it is consistent with other API like Dataset.set_xindex() and Dataset.xindexes.

I also refactored the API reference documentation so Coordinates has its own expanded section with subsections similar to the ones of the Dataset, DataArray and DataTree sections. The Coordinates class likely won't be used as much as the other ones, but since it has some utility and it is public API I thought it'd be worth better expose it.

  • Tests added
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

Make it more consistent with ``Dataset``, ``DataArray`` and
``DataTree``. The ``Coordinates`` class is 2nd order compared to the
former ones, but it is public API and useful (for creating coordinates
from indexes and merging coordinates together) so it deserves its own
(expanded) section + summary tables in the API reference doc.
@benbovy
Copy link
Member Author

benbovy commented Jan 29, 2025

Ticket number #10000, nice! 🎉

@@ -352,6 +353,35 @@ def _construct_direct(
)
return obj

@classmethod
def from_xindex(cls, index: Index) -> Self:
Copy link
Member

Choose a reason for hiding this comment

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

Seems a little inconsistent that the signature of this method only accepts one index, when the __init__ accepts multiple?

I'm also a bit unclear why you need to pass a Mapping from str names to indexes to __init__ but this method apparently doesn't require names?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes I agree it is confusing and I'll clarify this.

__init__ is the low-level(-ish) constructor where we can pass any arbitrary mapping of coordinate variables (and optionally a mapping of Xarray indexes) that are just passed to a direct constructor with only little safety checks, whereas from_xindex creates a new Coordinates object from exactly one Xarray index via creating the variables and indexes mappings from the index object.

For example with a custom coordinate transform index (#9543):

class CustomIndex(CoordinateTransformIndex):
    ...

index = CustomIndex(...)

writing

coords = Coordinates.from_xindex(index)

is equivalent but safer and more convenient than writing

vars = index.create_variables()
indexes = {name: index for name in vars}
coords = Coordinates(coords=vars, indexes=indexes)

Alternatively to Coordinates.from_xindex() we could provide more specific constructors like Coordinates.from_transform() (alongside the already existing Coordinates.from_pandas_multiindex()), but Coordinates.from_xindex() is more generic and future-proof, i.e., it will just work with any custom index that can be created from anything other than pre-existing Xarray coordinates.

Copy link
Member

Choose a reason for hiding this comment

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

Okay thank you for the explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

3 participants