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

prefer the objects _repr_inline_ over xarray's custom reprs #5352

Merged
merged 1 commit into from
May 25, 2021

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented May 19, 2021

In preparation of pushing these upstream, this increases the priority of the _repr_inline_ implementation of dask or sparse (once they are implemented) over our own custom functions.

  • Tests added
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst

@keewis
Copy link
Collaborator Author

keewis commented May 20, 2021

I don't think we need tests (or a whats-new) for this so this should be ready for merging. The only issue notable change I think users can see is that when (if?) dask accepts the _repr_inline_, they will probably use chunktype instead of meta (because that's what their repr uses).

@max-sixty max-sixty added the plan to merge Final call for comments label May 20, 2021
@shoyer
Copy link
Member

shoyer commented May 21, 2021

I wonder if _repr_inline_ should include something explicit about excluded fields in the protocol, e.g., obj._repr_inline_(shape=False, dtype=False) and/or if the name should make it clearer that it's for array objects, e.g., _ndarray_repr_inline_?

@keewis
Copy link
Collaborator Author

keewis commented May 22, 2021

hmm... adding shape and dtype as parameters would mean that we expect cases where we actually want to include either of those and where the normal repr is not enough. Do you have anything in mind for that?

I'm also not quite sure I get your point about the name: xarray currently does not allow wrapping anything other than duck arrays so I'm inclined to just not worry about this until that changes or someone else wants to use that name for a different purpose.

@shoyer
Copy link
Member

shoyer commented May 22, 2021

It's more that _repr_inline_ sounds very generic, neither really specific to xarray or even multi-dimensional arrays. So if I saw that without knowing the context of how it is used in xarray, it would surprise me that it's supposed to omit shape and dtype, which are otherwise essential parts of the repr.

Other possibilities:

  • Give the protocol a namespace, like _xarray_repr_inline_ to make it clearer that it's for Xarray's use.
  • Maybe add an omit argument, e.g., obj._repr_inline_(max_width, skip=['dtype', 'shape']). Attributes in skip are already represented elsewhere and can be skipped if desired.

@keewis
Copy link
Collaborator Author

keewis commented May 24, 2021

I see your points. I chose the name as _repr_inline_ because that seemed the most straight-forward but didn't think too deeply about this. If we do change the name I would like to keep the name as _repr_*_. Maybe _repr_array_inline_?

However, as I said I don't think it matters if the name is reused in a different context, as long as that is not incompatible with our use: we only care about arrays. Which is why I would slightly prefer the second alternative (a generic name and a parameter which is interpreted depending on the context). The only issue I have is that it's usually not a good idea to extend the API unless there's a specific need so I'm somewhat reluctant to add that parameter.

@keewis
Copy link
Collaborator Author

keewis commented May 24, 2021

in any case, I think any changes to the signature of _repr_inline_ should be a separate PR

@shoyer
Copy link
Member

shoyer commented May 25, 2021

in any case, I think any changes to the signature of _repr_inline_ should be a separate PR

I agree, let's discuss this in #5372

@keewis keewis merged commit 55e5b5a into pydata:master May 25, 2021
@keewis keewis deleted the _repr_inline_ branch May 25, 2021 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants