-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 options to control expand/collapse of sections in display of Dataset and DataArray #5126
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a nice complete solution to me! I just found the one typo :)
Thanks for the review @jsignell! |
@tomwhite would you like to add a note to whatsnew, and then we can merge? |
Co-authored-by: Julia Signell <jsignell@gmail.com>
@max-sixty yes - I've rebased and updated whats-new. |
Great — looks like one small linting issue, and then ping when green and we can hit the big button |
that linting issue |
Thanks for the PR @tomwhite , we're happy to have you as a contributor! |
Thank you! |
if _get_boolean_with_default("display_expand_data", default=True): | ||
data_repr = short_data_repr(arr) | ||
else: | ||
data_repr = inline_variable_array_repr(arr, OPTIONS["display_width"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this raises for
ds = xr.tutorial.open_dataset("rasm")
with xr.set_options(display_expand_data=False):
display(ds.Tair)
i.e. with the data being a MemoryCachedArray
. Should this be something like
data_repr = inline_variable_array_repr(arr, OPTIONS["display_width"]) | |
data_repr = inline_variable_array_repr(arr.variable, OPTIONS["display_width"]) |
or should we let display_expand_data
have no effect for MemoryCachedArray
(the expanded repr
is already really small)? The difference would be: [2029500 values with dtype=float64]
for display_expand_data=True
vs ...
for display_expand_data=False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's more helpful to always have the expanded repr
, but I'm not sure how complicated that special-case would be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for finding this @keewis. I can put together a PR - I'm just trying to find a simple test case that exposes the problem (the tests don't currently use the tutorial as far as I can tell).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should work with any opened file, so the test case should use either open_dataset
or open_dataarray
(creating the MemoryCachedArray
by hand would also work but might be a bit more difficult). Something like
@requires_netCDF4
def test_repr_memory_cached_collapsed(temp_path):
arr = xr.DataArray([0, 1], dims="x")
arr.to_netcdf(temp_path, engine="netcdf4")
with xr.open_dataarray(temp_path) as arr, xr.set_options(display_expand_data=True):
# check the result of repr
...
might work? (temp_path
should be a builtin pytest fixture to create a temporary file and return the path, but I'm not sure I got the name right)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've opened #5235 with a possible fix.
pre-commit run --all-files
whats-new.rst
api.rst