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.

Sign up for GitHub

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

Jump to bottom

allow customizing the inline repr of a duck array #4248

Merged
merged 13 commits into from
Aug 6, 2020
Merged

allow customizing the inline repr of a duck array #4248

merged 13 commits into from
Aug 6, 2020

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented Jul 22, 2020 •
edited
Loading

This calls the duck array's _repr_short_ (does not have to be that name, might also be _repr_inline_ or something else) if it exists, which can make e.g. pint.Quantity's inline repr a lot more useful. We might also be able to use that to get rid of our inline_dask_repr and inline_sparse_repr functions by pushing that upstream (or by monkeypatching).

@jthielen jthielen mentioned this pull request Jul 22, 2020
Merged
5 tasks
@max-sixty
Copy link
Collaborator

Looks great to me.

I'm not sure the naming convention: is a single _ dunder standard? Or does that look like a private method?

@jthielen
Copy link
Contributor

jthielen commented Aug 5, 2020 •
edited
Loading

I think _repr_*_ is the standard from IPython/Jupyter (https://ipython.readthedocs.io/en/stable/config/integrating.html), so _repr_short_ or _repr_inline_ is just following along with that.

@keewis
Copy link
Collaborator Author

keewis commented Aug 5, 2020

I think _repr_*_ is the standard from IPython/Jupyter

exactly, that's where the name came from.

Also, if consistency with the functions in xarray.core.formatting is important it might be better to choose _repr_inline_: the short_*_repr functions simply shorten the normal repr.

@keewis
Copy link
Collaborator Author

keewis commented Aug 5, 2020

if I remember correctly, this was accepted in this week's community meeting. Should this be merged, then?

@max-sixty
Copy link
Collaborator

max-sixty commented Aug 5, 2020 •
edited
Loading

LGTM! A test wouldn't go amiss but fine to keep moving given the size of the change imo

Thank you @keewis !

@keewis
Copy link
Collaborator Author

keewis commented Aug 5, 2020

tests are a good point, and I guess this should also be documented somewhere. Any ideas where that could be? Internals?

@dcherian
Copy link
Contributor

dcherian commented Aug 5, 2020

"internals" sounds good to me. It could also go in the future NEP-18 wrapping section.

@keewis
Copy link
Collaborator Author

keewis commented Aug 5, 2020

I put it into internals for now, together with a small paragraph about the requirements on duck arrays, but this should definitely be extended and moved somewhere more visible – maybe a Extending xarray document, together with the current section about custom accessors?

I also added a test, but it feels a bit strange to compare the result with the return value of a direct call to the object's _repr_inline_.

dcherian
dcherian approved these changes Aug 5, 2020
View reviewed changes
doc/internals.rst Outdated Show resolved Hide resolved
@max-sixty max-sixty self-requested a review August 6, 2020 00:09
dcherian
dcherian reviewed Aug 6, 2020
View reviewed changes
doc/whats-new.rst Outdated Show resolved Hide resolved
@keewis
Copy link
Collaborator Author

keewis commented Aug 6, 2020

should be ready for review again

@max-sixty
Copy link
Collaborator

Perfect! Thanks for adding to the docs!

@dcherian
Copy link
Contributor

dcherian commented Aug 6, 2020

Thanks @keewis

get rid of our inline_dask_repr and inline_sparse_repr functions by pushing that upstream

I think this is a great idea!

@dcherian dcherian merged commit 7ba19e1 into pydata:master Aug 6, 2020
@keewis keewis deleted the duckarray-short-repr branch August 6, 2020 15:50
This was referenced Aug 6, 2020
Closed
Open
shoyer
shoyer reviewed Aug 6, 2020
View reviewed changes
doc/internals.rst
Comment on lines +56 to +61
In certain situations (e.g. when printing the collapsed preview of
variables of a ``Dataset``), xarray will display the repr of a `duck array`_
in a single line, truncating it to a certain number of characters. If that
would drop too much information, the `duck array`_ may define a
``_repr_inline_`` method that takes ``max_width`` (number of characters) as an
argument:
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to add examples of what should and should not be included in _repr_inline_.

In particular, shape and dtype should not be included because xarray already adds those in a standard way.

For examples of what good _inline_repr_ look like, we could show examples of the output for dask and sparse.

@keewis keewis mentioned this pull request Aug 6, 2020
Merged
@mathause mathause mentioned this pull request Aug 13, 2020
Merged
4 tasks
dcherian added a commit to rpgoldman/xarray that referenced this pull request Aug 14, 2020
@dcherian
Merge branch 'master' of github.com:pydata/xarray into drop-vars-doc
f648f41 
* 'master' of github.com:pydata/xarray: (260 commits)
  Increase support window of all dependencies (pydata#4296)
  Implement interp for interpolating between chunks of data (dask) (pydata#4155)
  Add @mathause to current core developers. (pydata#4335)
  install sphinx-autosummary-accessors from conda-forge (pydata#4332)
  Use sphinx-accessors-autosummary (pydata#4323)
  ndrolling fixes (pydata#4329)
  DOC: fix typo argmin -> argmax in DataArray.argmax docstring (pydata#4327)
  pin sphinx to 3.1(pydata#4326)
  nd-rolling (pydata#4219)
  Implicit dask import 4164 (pydata#4318)
  allow customizing the inline repr of a duck array (pydata#4248)
  silence the known docs CI issues (pydata#4316)
  enh: fixed pydata#4302 (pydata#4315)
  Remove all unused and warn-raising methods from AbstractDataStore (pydata#4310)
  Fix map_blocks example (pydata#4305)
  Fix docstring for missing_dims argument to isel methods (pydata#4298)
  Support for PyCharm remote deployment (pydata#4299)
  Update map_blocks and map_overlap docstrings (pydata#4303)
  Lazily load resource files (pydata#4297)
  warn about the removal of the ufuncs (pydata#4268)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Aug 15, 2020
@dcherian
Merge remote-tracking branch 'upstream/master' into propagate-attrs
b86623c 
* upstream/master: (34 commits)
  Fix bug in computing means of cftime.datetime arrays (pydata#4344)
  fix some str accessor inconsistencies (pydata#4339)
  pin matplotlib in ci/requirements/doc.yml (pydata#4340)
  Clarify drop_vars return value. (pydata#4244)
  Support explicitly setting a dimension order with to_dataframe() (pydata#4333)
  Increase support window of all dependencies (pydata#4296)
  Implement interp for interpolating between chunks of data (dask) (pydata#4155)
  Add @mathause to current core developers. (pydata#4335)
  install sphinx-autosummary-accessors from conda-forge (pydata#4332)
  Use sphinx-accessors-autosummary (pydata#4323)
  ndrolling fixes (pydata#4329)
  DOC: fix typo argmin -> argmax in DataArray.argmax docstring (pydata#4327)
  pin sphinx to 3.1(pydata#4326)
  nd-rolling (pydata#4219)
  Implicit dask import 4164 (pydata#4318)
  allow customizing the inline repr of a duck array (pydata#4248)
  silence the known docs CI issues (pydata#4316)
  enh: fixed pydata#4302 (pydata#4315)
  Remove all unused and warn-raising methods from AbstractDataStore (pydata#4310)
  Fix map_blocks example (pydata#4305)
  ...
dcherian added a commit to dcherian/xarray that referenced this pull request Aug 16, 2020
@dcherian
Merge remote-tracking branch 'upstream/master' into fix/4107
1ea8666 
* upstream/master: (40 commits)
  Fix bug in computing means of cftime.datetime arrays (pydata#4344)
  fix some str accessor inconsistencies (pydata#4339)
  pin matplotlib in ci/requirements/doc.yml (pydata#4340)
  Clarify drop_vars return value. (pydata#4244)
  Support explicitly setting a dimension order with to_dataframe() (pydata#4333)
  Increase support window of all dependencies (pydata#4296)
  Implement interp for interpolating between chunks of data (dask) (pydata#4155)
  Add @mathause to current core developers. (pydata#4335)
  install sphinx-autosummary-accessors from conda-forge (pydata#4332)
  Use sphinx-accessors-autosummary (pydata#4323)
  ndrolling fixes (pydata#4329)
  DOC: fix typo argmin -> argmax in DataArray.argmax docstring (pydata#4327)
  pin sphinx to 3.1(pydata#4326)
  nd-rolling (pydata#4219)
  Implicit dask import 4164 (pydata#4318)
  allow customizing the inline repr of a duck array (pydata#4248)
  silence the known docs CI issues (pydata#4316)
  enh: fixed pydata#4302 (pydata#4315)
  Remove all unused and warn-raising methods from AbstractDataStore (pydata#4310)
  Fix map_blocks example (pydata#4305)
  ...
This was referenced Sep 14, 2020
Open
Open
@keewis keewis mentioned this pull request Oct 7, 2020
Open
@keewis keewis mentioned this pull request Feb 16, 2021
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcherian dcherian dcherian approved these changes

@shoyer shoyer shoyer left review comments

@max-sixty max-sixty max-sixty approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature request: show units in dataset overview
5 participants
@keewis @max-sixty @jthielen @dcherian @shoyer