Skip to content

Improve discoverability of index build options #8002

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

Open
benbovy opened this issue Jul 19, 2023 · 2 comments
Open

Improve discoverability of index build options #8002

benbovy opened this issue Jul 19, 2023 · 2 comments

Comments

@benbovy
Copy link
Member

benbovy commented Jul 19, 2023

Is your feature request related to a problem?

Currently Dataset.set_xindex(coord_names, index_cls=None, **options) allows passing index build options (if any) via the **options arguments. Those options are not easily discoverable, though (no auto-completion, etc.).

Describe the solution you'd like

What about something like this?

ds.set_xindex("x", MyCustomIndex.with_options(foo=1, bar=True))

# or

ds.set_xindex("x", *MyCustomIndex.with_options(foo=1, bar=True))

This would require adding a .with_options() class method that can be overridden in Index subclasses (optional):

# xarray.core.indexes

class Index:
    @classmethod
    def with_options(cls) -> tuple[type[Self], dict[str, Any]]:
        return cls, {}
# third-party code
from xarray.indexes import Index

class MyCustomIndex(Index):

    @classmethod
    def with_options(cls, foo: int = 0, bar: bool = False) -> tuple[type[Self], dict[str, Any]]:
        """Set a new MyCustomIndex with options.

        Parameters
        ------------
        foo : int, optional
            The foo option (default: 1).
        bar : bool, optional
            The bar option (default: False).
        """
        return cls, {"foo": foo, "bar": bar}

Thoughts?

Describe alternatives you've considered

Build options are also likely defined in the Index constructor, e.g.,

# third-party code
from xarray.indexes import Index

class MyCustomIndex(Index):
    
    def __init__(self, data, foo=0, bar=False):
        ...

However, the Index constructor is not public API (only used internally and indirectly in Xarray when setting a new index from existing coordinates).

Any other idea?

Additional context

No response

@TomNicholas
Copy link
Member

So if I understand this correctly the advantage of with_options is that is would allow the author of the custom index to define the signature of additional arguments it expects? I think that's useful.

I also think it's generally a bit confusing that in all these index examples __init__ is defined but not actually how the index is first created. I understand why, but the intended patterns could be more clearly documented.

@benbovy
Copy link
Member Author

benbovy commented Jul 19, 2023

So if I understand this correctly the advantage of with_options is that is would allow the author of the custom index to define the signature of additional arguments it expects?

Yes and it would expose the signature directly to the users of the index (type checking, auto-completion, etc.).

I also think it's generally a bit confusing that in all these index examples init is defined but not actually how the index is first created. I understand why, but the intended patterns could be more clearly documented.

Yeah this may be confusing I agree. And I also agree about clarifying it in the documentation.

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

No branches or pull requests

2 participants