provide set_option `collapse_html` to control HTML repr collapsed state #4230

michaelaye · 2020-07-16T02:29:07Z

Closes FR: Provide option for collapsing the HTML display in notebooks #4229
Tests added
Passes isort -rc . && black . && mypy . && flake8
User visible changes (including notable bug fixes) are documented in whats-new.rst

michaelaye · 2020-07-16T02:33:56Z

I didn't isort this commit, usually other peeps hate it when i do it, happy that you actually require it! :)

But I'm getting this return while using isort -rc . in the root repo folder (is that the idea?)

$ isort -rc .                                                                                      (xarray-tests) 
/home/maye/miniconda3/envs/xarray-tests/lib/python3.7/site-packages/setuptools/distutils_patch.py:26: UserWarning: Distutils was imported before Setuptools. This usage is discouraged and may exhibit undesirable behaviors or errors. Please use Setuptools' objects directly or at least import Setuptools first.
  "Distutils was imported before Setuptools. This usage is discouraged "
Skipped 4 files

michaelaye · 2020-07-16T02:37:32Z

For mypy I'm getting:

. is not a valid Python package name

michaelaye · 2020-07-16T02:55:25Z

Oh, implemented the wrong default, things were confusing as the HTML code "checked" for the collapsed variable apparently means NOT collapsed...

michaelaye · 2020-07-16T03:27:43Z

I'm looking at the relevant test function:

def test_repr_of_dataarray(dataarray):
    formatted = fh.array_repr(dataarray)
    assert "dim_0" in formatted
    # has an expanded data section
    assert formatted.count("class='xr-array-in' type='checkbox' checked>") == 1
    # coords and attrs don't have an items so they'll be be disabled and collapsed
    assert (
        formatted.count("class='xr-section-summary-in' type='checkbox' disabled >") == 2
    )

I'm not very good with HTML, does this only check on the number of collapsed sections? I am kinda guessing that this code is abusing checkbox code to make the display collapsible? How could I check specifically if the data section is collapsed based on the OPTION setting?

jsignell · 2020-07-21T13:50:26Z

xarray/core/options.py

@@ -21,6 +22,7 @@
    CMAP_DIVERGENT: "RdBu_r",
    KEEP_ATTRS: "default",
    DISPLAY_STYLE: "html",
+    COLLAPSE_HTML: True,


This should be False by default to match existing behavior

jsignell · 2020-07-21T13:52:13Z

xarray/core/formatting_html.py

@@ -184,7 +185,7 @@ def dim_section(obj):
 def array_section(obj):
    # "unique" id to expand/collapse the section
    data_id = "section-" + str(uuid.uuid4())
-    collapsed = "checked"
+    collapsed = "" if OPTIONS["collapse_html"] else "checked"


I think you are right that this collapsed var is misnamed. It would be clearer to rename it to "expanded". The collapsible_section also has this inverted name.

i can rename it in the array section but I guess renaming it in the collapsible_section leaves the scope of this issue?

problem is that for collapsible_section it's a function parameter, so it would need to be changed in all calling cases as well for clarity?

jsignell · 2020-07-21T13:57:19Z

I'm looking at the relevant test function:

def test_repr_of_dataarray(dataarray):
    formatted = fh.array_repr(dataarray)
    assert "dim_0" in formatted
    # has an expanded data section
    assert formatted.count("class='xr-array-in' type='checkbox' checked>") == 1
    # coords and attrs don't have an items so they'll be be disabled and collapsed
    assert (
        formatted.count("class='xr-section-summary-in' type='checkbox' disabled >") == 2
    )

does this only check on the number of collapsed sections?

Yes, that test counts expanded data sections and collapsed summary sections

How could I check specifically if the data section is collapsed based on the OPTION setting?

Use set_option as a context manager and do a test much like the one above.

pep8speaks · 2020-08-01T07:33:36Z

Hello @michaelaye! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-02-27 20:54:05 UTC

shoyer · 2020-08-01T23:48:07Z

I support adding an option for this, but I think a name like html_expand_data or html_collapse_data would be more descriptive, because this refer specifically to the "data" section of HTML reprs.

I also agree with @jsignell that the current naming seems to be reversed "expanded" should mean show the data section and "collapsed" should mean hiding it.

michaelaye · 2020-08-02T02:32:26Z

i'm fine in fixing the name of the variable, but usually open source teams don't want a PR that addresses more than one issue at a time, are you sure you want it in here?

shoyer · 2020-08-02T03:45:40Z

The scope is limited enough here that it would be fine to include the cleanup in this PR. The bigger issue is that names exposed in a public API need to be chosen much more carefully than internal only names — and we definitely don’t want to expose the current poorly chosen internal name for this feature to users.

…

On Sat, Aug 1, 2020 at 7:32 PM Michael Aye ***@***.***> wrote: i'm fine in fixing the name of the variable, but usually open source teams don't want a PR that addresses more than one issue at a time, are you sure you want it in here? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4230 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJJFVTTULSMPGZWCUNIGPTR6TF4LANCNFSM4O3LS7JA> .

benbovy · 2020-09-21T23:01:09Z

but I think a name like html_expand_data or html_collapse_data would be more descriptive, because this refer specifically to the "data" section of HTML reprs.

Do we want that this expand/collapse option affects the text repr too (i.e., collapsed = show inline values repr vs. expanded = show the whole data repr)? This might be more consistent, although obviously there's no way to interactively expand/collapse the data section in the text repr.

In that case display_expand_data or display_collapse_data would be better names (using the same prefix than the display_style option).

tomwhite · 2021-04-06T11:54:06Z

I would find it useful to have a generalized version of this for datasets, so it's possible to control independently whether the coordinates, data variables, and attributes sections are expanded by default.

I'd be happy to pick this up if no one else is working on it. I have created a branch to try it out, which adds the options display_expand_attrs, display_expand_coords, display_expand_data, and display_expand_data_vars for controlling both HTML and text representations. All default to True.

One thing I'm not sure about is maintaining the existing behaviour, which is that for the HTML dataset repr all three sections are expanded by default, up to a certain size limit, depending on the section (25 coordinates, 15 data variables, 10 attributes).
I couldn't see a simple way to incorporate that, although maybe it's not needed if the sections can be collapsed by setting an option (e.g. xr.set_options(display_expand_attrs=False))?

dcherian · 2021-04-06T16:24:06Z

maintaining the existing behaviour, which is that for the HTML dataset repr all three sections are expanded by default,

The existing behaviour could be "default", True could be fully expanded always; and False could be never expanded.

display_expand_attrs, display_expand_coords, display_expand_data, and display_expand_data_vars f

These are a lot with potentially two more: indexes and encoding.

Should we instead do something like

xr.set_options(display_expand = {
"data_vars": False,
"coords": "default",
"attrs": False,
})

tomwhite · 2021-04-07T10:51:31Z

Thanks for the suggestions @dcherian!

I've implemented "default" to preserve existing behaviour (i.e. only collapse if greater than preconfigured size limits). See #5126

I haven't implemented the nested options for display_expand - is it worth adding the extra complexity here?

dcherian · 2021-04-19T02:08:49Z

I haven't implemented the nested options for display_expand - is it worth adding the extra complexity here?

Not sure :). Can you open a PR please?

EDIT: Oops you've already opened one! thanks!

dcherian · 2021-05-13T17:02:54Z

I think @tomwhite has implemented this functionality. Thanks everyone.

provide set_option collapse_html to control HTML repr collapsed state

8a591c6

fix isort

6a5c475

jsignell reviewed Jul 21, 2020

View reviewed changes

mathause mentioned this pull request Jul 30, 2020

unpin docs again? regionmask/regionmask#165

Closed

michaelaye added 3 commits July 31, 2020 19:44

fix: change default to match existing behavior

cb179a3

fix: fix tests for collapse_html option

e5b6246

docs: add "new feature" entry for new collapse_html option

33e40cf

style: fix PEP8

a1082da

michaelaye marked this pull request as ready for review August 1, 2020 07:36

michaelaye marked this pull request as draft August 4, 2020 23:10

dcherian mentioned this pull request Sep 21, 2020

Pre-expand data and attributes in DataArray/Variable HTML repr? #4176

Closed

aarondewindt mentioned this pull request Sep 21, 2020

Add option to Pre-expand or collapse data and attributes in DataArray/Variable HTML repr. #4446

Closed

Merge branch 'master' into html_repr_option

6c1e261

dcherian closed this May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provide set_option `collapse_html` to control HTML repr collapsed state #4230

provide set_option `collapse_html` to control HTML repr collapsed state #4230

michaelaye commented Jul 16, 2020 •

edited

Loading

michaelaye commented Jul 16, 2020 •

edited

Loading

michaelaye commented Jul 16, 2020

michaelaye commented Jul 16, 2020

michaelaye commented Jul 16, 2020

jsignell Jul 21, 2020

jsignell Jul 21, 2020

michaelaye Aug 1, 2020

michaelaye Aug 1, 2020

jsignell commented Jul 21, 2020 •

edited

Loading

pep8speaks commented Aug 1, 2020 •

edited

Loading

shoyer commented Aug 1, 2020

michaelaye commented Aug 2, 2020

shoyer commented Aug 2, 2020 via email

benbovy commented Sep 21, 2020

tomwhite commented Apr 6, 2021

dcherian commented Apr 6, 2021

tomwhite commented Apr 7, 2021

dcherian commented Apr 19, 2021 •

edited

Loading

dcherian commented May 13, 2021

provide set_option collapse_html to control HTML repr collapsed state #4230

provide set_option collapse_html to control HTML repr collapsed state #4230

Conversation

michaelaye commented Jul 16, 2020 • edited Loading

michaelaye commented Jul 16, 2020 • edited Loading

michaelaye commented Jul 16, 2020

michaelaye commented Jul 16, 2020

michaelaye commented Jul 16, 2020

jsignell Jul 21, 2020

Choose a reason for hiding this comment

jsignell Jul 21, 2020

Choose a reason for hiding this comment

michaelaye Aug 1, 2020

Choose a reason for hiding this comment

michaelaye Aug 1, 2020

Choose a reason for hiding this comment

jsignell commented Jul 21, 2020 • edited Loading

pep8speaks commented Aug 1, 2020 • edited Loading

Comment last updated at 2021-02-27 20:54:05 UTC

shoyer commented Aug 1, 2020

michaelaye commented Aug 2, 2020

shoyer commented Aug 2, 2020 via email

benbovy commented Sep 21, 2020

tomwhite commented Apr 6, 2021

dcherian commented Apr 6, 2021

tomwhite commented Apr 7, 2021

dcherian commented Apr 19, 2021 • edited Loading

dcherian commented May 13, 2021

provide set_option `collapse_html` to control HTML repr collapsed state #4230

provide set_option `collapse_html` to control HTML repr collapsed state #4230

michaelaye commented Jul 16, 2020 •

edited

Loading

michaelaye commented Jul 16, 2020 •

edited

Loading

jsignell commented Jul 21, 2020 •

edited

Loading

pep8speaks commented Aug 1, 2020 •

edited

Loading

dcherian commented Apr 19, 2021 •

edited

Loading