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

Altair should accept iterables where currently list/tuple/ndarray is required #2877

Closed
davidgilbertson opened this issue Feb 7, 2023 · 2 comments

Comments

@davidgilbertson
Copy link

My scenario: I want the x-axis of my chart to show values in increments of 7 (autocorrelation plot). I do this by providing the tick values explicitly:

alt.X(
    "lag:Q",
    axis=alt.Axis(
        values=range(0, len(chart_df), 7),
    ),
)

This errors, because range is not a list, and this line in Altair checks for quite specific types

    elif isinstance(obj, (list, tuple, np.ndarray)):
        return [_todict(v, validate, context) for v in obj]

I think this is overly restrictive, this should be able to handle any sort of iterable (generators, sets, iterators, etc)

Some options:

  • Check for isinstance(obj, collections.abc.Iterable) - this check should come last because dicts are iterable.
  • Check for isinstance(obj, collections.abc.Sequence) - but beware this doesn't include Numpy arrays.

IMO Iterable (docs) is the way to go.

Also, if you wanted to handle lots of other data types without much effort (e.g. PyTorch and TensorFlow tensors) you could do what Matplotlib do and cast everything to NumPy arrays with nd.array(iterable). That way NumPy does all the work (checking for __array__ methods, etc) and I would expect it to be pretty robust.

You could even type these as ArrayLike, borrowing from NumPy, similar to what Pandas does.

@joelostblom
Copy link
Contributor

Thanks for raising this, I agree that it would be useful to be more flexible in what types of iterables that are accepted at various places in Altair. I like the idea of using the same approach as matplotlib as this would already have been battle tested and work well for them and we just need to make sure it also works here. A PR here would be welcome and we can continue discussing there once we see what a solution to this could look like. Related #2808

@joelostblom
Copy link
Contributor

fixed in #3501

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