Uh oh!
There was an error while loading. Please reload this page.
Html repr - #3425
Conversation
jsignell
commented
Oct 21, 2019
I don't really like how the |
dcherian
commented
Oct 21, 2019
Honestly this is looking super good and ready to merge already! Thanks @jsignell The only thing that looks funny to me is the alternating black-gray text coloring below: |
jsignell
commented
Oct 22, 2019
Yeah I agree that is a strange part of the original PR. The version in the fiddle doesn't do that, so I'd really like to hear back from @benbovy about whether the fiddle css is more uptodate. |
rabernat
commented
Oct 22, 2019
Julia, it's fantastic to see you working on this! Thanks so much! |
shoyer
commented
Oct 22, 2019
Uh oh!
There was an error while loading. Please reload this page.
| d['id'] = 'section-' + str(uuid.uuid4()) | ||
| # TODO: no value preview if not in memory | ||
| d['preview'] = format_values_preview(obj.values, max_char=70) |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
benbovy
commented
Oct 22, 2019
This is really nice @jsignell !
We did a lot of experiments, even some after starting the implementation in #1820, so I admit this is quite messy. I also did some clean-up and tweaks directly in that PR, so unfortunately there is no fiddle of reference that exactly corresponds to what's in here. Sorry for that! The html/css in this PR is the cleanest one, I think. The latest fiddles (for
In the HTML repr, the width of the space character is less large that for the monospace font used in the text repr, so I originally used this trick to better distinguish between the values in the inline data repr. Now that each value is encapsulated in it's own |
jsignell
commented
Oct 22, 2019
Yeah the data preview is also shifted vertically. |
jsignell
commented
Oct 22, 2019
I just updated the gist with these changes:
|
Uh oh!
There was an error while loading. Please reload this page.
jsignell
commented
Oct 22, 2019
Is there ever a situation in which dimensions would be expandable? It seems like it is hard-coded to collapsed. I am trying to generalize the css to reduce the length. |
shoyer
commented
Oct 22, 2019
I don't think it would ever be expanded. The collapsed icon is just there for visual consistency. |
jsignell
commented
Oct 22, 2019
Ok I think I've addressed all the comments. I just updated the gist with the latest output. Tomorrow I'll start adding tests. The css is now ~300 lines and it gets injected everytime the dataset is rendered. I'm not sure if that is considered too long and if it is I don't quite know what to do about it. |
shoyer
commented
Oct 22, 2019
You could consider running the CSS through some sort of preprocessing tool to minify it, e.g., to remove all the spaces. But my guess is that it's probably fine. It would be informative to measure the size of the output HTML encoded as UTF-8 bytes for some example datasets. For context, a simple line chart from matplotlib is ~20 KB when saved as a PNG image. Anything in that ball-park would be OK, up to perhaps a few hundred KB for really big datasets with lots of variables. |
| def _repr_html_(self): | ||
| if OPTIONS["display_style"] == "classic": | ||
| classic = repr(self).replace("<", "<").replace(">", ">") |
There was a problem hiding this comment.
Use html.escape here: https://docs.python.org/3/library/html.html#html.escape
| def _repr_html_(self): | ||
| if OPTIONS["display_style"] == "classic": | ||
| classic = repr(self).replace("<", "<").replace(">", ">") | ||
| return "<pre>{repr}</pre>".format(repr=classic) |
There was a problem hiding this comment.
Could use an f-string here:
| return"<pre>{repr}</pre>".format(repr=classic) | |
| returnf"<pre>{classic}</pre>" |
There was a problem hiding this comment.
I didn't realize that xarray was already supporting >=3.6 👍
Uh oh!
There was an error while loading. Please reload this page.
shoyer
commented
Oct 23, 2019
We probably should be careful to use |
Codecov Report
@@ Coverage Diff @@## master #3425 +/- ##
=========================================
- Coverage 96.16% 95.06% -1.1%
=========================================
Files 65 66 +1 Lines 13436 13587 +151 =========================================
- Hits 12921 12917 -4 - Misses 515 670 +155
Continue to review full report at Codecov.
|
shoyer
commented
Oct 23, 2019
jsignell
commented
Oct 23, 2019
I'm still worried about size, but minifying the css and even minifying the html did not make a big difference. It looks like the html_repr for rasm is 38kb . |
shoyer
commented
Oct 23, 2019
How are you measuring this? I see: |
|
jsignell
commented
Oct 23, 2019
Ok I am going to write some tests now. I updated the gist. Do people like the names "classic" and "html" for the display_style options? Another option would be "classic" and "rich". |
benbovy
commented
Oct 23, 2019
Or maybe "text" and "html"? Hopefully "html" will eventually become the classic display :) |
jsignell
commented
Oct 23, 2019
I added some tests. I can add more if anyone has a test case that they want to be sure is covered. |
jsignell
commented
Oct 24, 2019
Ok I think it's fixed now |
benbovy
commented
Oct 24, 2019
Yes it looks nice! |
jsignell
commented
Oct 24, 2019
It isn't too hard to get it working with dark theme, but requires slight tweaks to the colors to align with those provided as vars by jlab. |
dcherian
commented
Oct 24, 2019
Awesome. Thanks @jsignell Let's leave dark theme compatibility for a future PR. |
shoyer
commented
Oct 24, 2019
Should we expand the |
benbovy
commented
Oct 24, 2019
Thanks a lot @jsignell ! It is great to see this merged finally! |
mrocklin
commented
Oct 24, 2019
Thanks @jsignell (and all). I'm really jazzed about this. |
this is awesome! I was so excited about it that I updated my little blog post with the latest xarray master https://predictablynoisy.com/xarray-explore-ieeg I was a little sad at the gigantic repr that came out of my DataArray, and now it is nice and tidy html! |
benbovy
commented
Oct 24, 2019
Cool! I guess this post about how you use notebooks in your blog is still up to date? I looks like we'll need to fix a couple of issues (the Besides variable names, we probably need to somehow handle overflow for variables that have a long list of dimension labels. |
Actually, no that's totally out of date now haha. Thanks for reminding me I should update that edit: realized I should have said what I'm actually using - I'm using this:https://jupyterbook.org/features/page.html |
jsignell
commented
Oct 24, 2019
I am opening a PR. I didn't escape dtypes and this dtype is |
shoyer
commented
Oct 24, 2019
To verify proper escaping, we could validate the generated HTML as XML: This could be a good use case for Hypothesis. Or you could manually build a dataset with variables of every dtype. |
jsignell
commented
Oct 24, 2019
I opened a PR and included a small test. It can probably go in while we work on a more complete testing infrastructure. |
jsignell
commented
Oct 24, 2019
These are handled in the same way as long variable names, they truncate with an ellipsis and then expand on hover. |
benbovy
commented
Oct 25, 2019
Oh yes I missed it. |
* upstream/master: Escaping dtypes (pydata#3444) Html repr (pydata#3425)
…e-multiple-dims * upstream/master: change ALL_DIMS to equal ellipsis (pydata#3418) Escaping dtypes (pydata#3444) Html repr (pydata#3425)
* upstream/master: Another groupby.reduce bugfix. (pydata#3403) add icomoon license (pydata#3448) change ALL_DIMS to equal ellipsis (pydata#3418) Escaping dtypes (pydata#3444) Html repr (pydata#3425)
* upstream/master: upgrade black verison to 19.10b0 (pydata#3456) Remove outdated code related to compatibility with netcdftime (pydata#3450) Remove deprecated behavior from dataset.drop docstring (pydata#3451) jupyterlab dark theme (pydata#3443) Drop groups associated with nans in group variable (pydata#3406) Allow ellipsis (...) in transpose (pydata#3421) Another groupby.reduce bugfix. (pydata#3403) add icomoon license (pydata#3448) change ALL_DIMS to equal ellipsis (pydata#3418) Escaping dtypes (pydata#3444) Html repr (pydata#3425)
commit bc39877 Merge: 507b1f6278d2e6 Author: dcherian <deepak@cherian.net> Date: Tue Oct 29 09:36:30 2019 -0600 Merge remote-tracking branch 'upstream/master' into dask-tokenize * upstream/master: upgrade black verison to 19.10b0 (pydata#3456) Remove outdated code related to compatibility with netcdftime (pydata#3450) Remove deprecated behavior from dataset.drop docstring (pydata#3451) jupyterlab dark theme (pydata#3443) Drop groups associated with nans in group variable (pydata#3406) Allow ellipsis (...) in transpose (pydata#3421) Another groupby.reduce bugfix. (pydata#3403) add icomoon license (pydata#3448) change ALL_DIMS to equal ellipsis (pydata#3418) Escaping dtypes (pydata#3444) Html repr (pydata#3425) commit 507b1f6 Author: dcherian <deepak@cherian.net> Date: Tue Oct 29 09:34:47 2019 -0600 Fix window test commit 4ab6a66 Author: dcherian <deepak@cherian.net> Date: Thu Oct 24 14:30:57 2019 -0600 Implement __dask_tokenize__






This PR supersedes #1820 - see that PR for original discussion. See this gist to try out the new MultiIndex and options functionality.
black . && mypy . && flake8whats-new.rstfor all changes andapi.rstfor new APITODO: