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

Change HTML repr away from LaTeX to a Sparse/Dask-like repr #1133

Merged
merged 1 commit into from
Aug 17, 2020

Conversation

jthielen
Copy link
Contributor

@jthielen jthielen commented Jul 14, 2020

While I've never been a big fan of Pint's LaTeX repr in JupyterLab/Jupyter Notebook due to working with large arrays that would crash the renderer, I've recently been working a lot with various wrapping combinations of xarray, Pint, Dask, and Sparse, in which Pint's LaTeX repr performs particularly poorly. Even if Pint's repr were elided (#765), it would still poorly nest with the other HTML reprs.

This motivated me to implement an alternative HTML repr for Pint using a simple HTML table similar to what both Dask and Sparse use. As shown in this demonstration notebook, this allows much better nesting of HTML reprs. A quick example image is below:

This doesn't quite take care of xarray's unit repr discussions (since this only shows up when expanding the data in a dataset), but I still think it is a worthwhile improvement for the expanded view. @keewis do you have any thoughts on this particular interaction?

Marking as a WIP/draft for now to see if this is a welcome format for the HTML repr first before I dig too much into optimizing the implementation and writing tests for it.

Tagging @nbren12, @hgrecco, @jondoesntgit, @natezb, @ipcoder, @keewis, and @TomNicholas for input based on past issues (#654, #765, and xarray-contrib/pint-xarray#6).

@keewis
Copy link
Contributor

keewis commented Jul 16, 2020

this would definitely be a big improvement of the expanded HTML repr of xarray objects (I never really looked at the repr of quantities in a notebook so I can't comment on that), although the repr for big numpy arrays looks a bit strange (to fix that, I guess numpy needs its own _repr_html_?). 👍 for the new HTML repr

To properly format the collapsed or text repr we'd need to first modify xarray, so I guess we can ignore them for now.

@keewis keewis mentioned this pull request Jul 17, 2020
16 tasks
@jthielen
Copy link
Contributor Author

jthielen commented Jul 22, 2020

Also, just to add a note here: assuming the _repr_short_ or _repr_inline_ of pydata/xarray#4248 goes through as brought up by @keewis, that would be important to add to Pint here or in a follow-up PR.

The API would be

_repr_inline_(max_width)

returning a string.

@jthielen
Copy link
Contributor Author

jthielen commented Aug 4, 2020

Pinging @hgrecco for your thoughts on this in particular as maintainer, since it's been three weeks since the initial inquiry? Thanks!

@keewis
Copy link
Contributor

keewis commented Aug 6, 2020

I've been wondering whether the _repr_inline_ would be a good fit for pint, or whether it should be provided by pint-xarray. Until it is adopted by other packages this would be something specific to xarray, so it would probably be better to implement in pint-xarray. However, that would mean we're relying on monkeypatching, which is generally not a good idea. Thoughts?

@jthielen
Copy link
Contributor Author

jthielen commented Aug 6, 2020

@keewis That's a good point.

I wouldn't be too concerned about monkeypatching for something like _repr_inline_, since it is a single method that can be injected into the global Quantity class, and as you said, is just used in this one place for xarray at the moment. I guess what it comes down to for me is: do we expect all users of xarray with pint to be always using pint-xarray? If so, then it definitely seems reasonable to monkeypatch this in pint-xarray. If not, it could be surprising behavior to the unaware user to have "nice" inline reprs when pint-xarray is available/imported and "ugly" reprs when not.

In any case, since it isn't quite the simple thing I initially thought, I think it would be best to defer implementation of _repr_inline_ to another PR (either direct implementation in Pint or monkeypatch in pint-xarray) so as to not cloud the more general HTML repr discussion here.

@keewis
Copy link
Contributor

keewis commented Aug 6, 2020

agreed. I'd say let's implement this in pint-xarray for now and if desired we can move it to pint later.

@jthielen
Copy link
Contributor Author

jthielen commented Aug 14, 2020

Just wanted to ping @hgrecco again on this. I'll be working on some xarray & Pint integration docs for MetPy in the near future, and it would be great to have this HTML repr in (or at have the general direction approved) by then.

@hgrecco
Copy link
Owner

hgrecco commented Aug 14, 2020

Sorry for the late reply,. pandemic times! I was also never really happy by the way pint represents this. So I agree with the change as long as an scalar is kept "normal" (as you also showed)!. Thanks for the PR (and pinging again!

@jthielen
Copy link
Contributor Author

Sorry for the late reply,. pandemic times! I was also never really happy by the way pint represents this. So I agree with the change as long as an scalar is kept "normal" (as you also showed)!. Thanks for the PR (and pinging again!

Sounds good, I'll work on the tests and then mark as ready for review!

@jthielen jthielen marked this pull request as ready for review August 16, 2020 15:24
@jthielen jthielen force-pushed the html-table-repr branch 3 times, most recently from 537ddaa to 93eeec4 Compare August 16, 2020 17:19
@jthielen jthielen changed the title WIP: Change HTML repr away from LaTeX to a Sparse/Dask-like repr Change HTML repr away from LaTeX to a Sparse/Dask-like repr Aug 16, 2020
@jthielen
Copy link
Contributor Author

Hopefully I've finally gotten the uncertainties stuff fixed, so (assuming it is) this should be ready for review and merge.

@hgrecco
Copy link
Owner

hgrecco commented Aug 17, 2020

bors r+

@hgrecco
Copy link
Owner

hgrecco commented Aug 17, 2020

Thanks for this PR!

@bors
Copy link
Contributor

bors bot commented Aug 17, 2020

Build succeeded:

@bors bors bot merged commit dea623a into hgrecco:master Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Turn off latex rendering in jupyter notebooks
3 participants