html representation for jupyter notebooks #1217

percygautam · 2020-06-01T12:09:14Z

Description

This PR implements the html representation for arviz.InferenceData object in jupyter notebooks. Related to discussion in #1066.

Checklist

Follows official PR format
Code style correct (follows pylint and black guidelines)
Changes are listed in changelog

arviz/data/inference_data.py

arviz/utils.py

canyon289

I may have a second round of comments after this first one. Also in regards to CI errors let us know if you need help learning how to get to them, or understand what they are saying.

Can you add a screenshot as well showing what these changes do in the notebook? I thought the xarray repr was already in html in jupyter if we set the xarray option. What is this adding on top of that?

OriolAbril

I would also update cookbook and quickstart notebooks to show the html goodness :)

arviz/data/inference_data.py

arviz/utils.py

percygautam · 2020-06-01T19:53:49Z

@canyon289 Yes the xarray's repr is in html if we set the option. This PR is making the whole inferenceData object dynamic. The result can be seen in this jupyter notebook: https://nbviewer.jupyter.org/github/percygautam/arviz-examples/blob/master/HTML%20repr.ipynb

OriolAbril · 2020-06-02T12:39:49Z

arviz/utils.py

+ </div>
+ """
+ element_template = """
+ <ul class="xr-sections">


Currently the vertical spacing between groups is very large. I think this could be due to ul being part of the element template as this is defining a different list for each group. Can you try moving it to the html template so that we have a single list (only one ul defined in html template) and the groups are then elements of this single list (one li per group)

This ul/li change is also marked as resolved but it is not and again I have a vague memory of seeing the ul moved to html instead of element template :/

canyon289

Havent had time to rereview but don't want to block if this PR is ready to go. Deferring judgement to whoever gets to it before me

OriolAbril · 2020-06-03T11:28:37Z

LGTM, we can merge after updating a couple notebooks.

canyon289 · 2020-06-03T13:35:54Z

I am going to request tests before merge. IMO all code should be tested if it can, and this is no exception :). I would add a test that calls _repr_html_ directly and tests that the html string is returned.

I would also add a tests the correct repr is returned without an explicit call. I assume this will require some mocking and to "trick" the non jupyter notebook test to provide the html repr.

As is such still requesting some changes :)

arviz/data/inference_data.py

canyon289 · 2020-06-03T13:40:06Z

arviz/utils.py

+
+
+class HtmlTemplate:
+ """Contains html templates."""


Expand docstring. Contains html templates for what?

Would change this to something along the lines of contains html templates for InferenceData repr

arviz/utils.py

canyon289 · 2020-06-03T13:44:50Z

@percygautam this is already looking great and youre on the right track. Hope you dont mind my additional comments :)

percygautam · 2020-06-04T19:52:52Z

I am going to request tests before merge. IMO all code should be tested if it can, and this is no exception :). I would add a test that calls _repr_html_ directly and tests that the html string is returned.

I would also add a tests the correct repr is returned without an explicit call. I assume this will require some mocking and to "trick" the non jupyter notebook test to provide the html repr.

I have added the basic test. How do I add the second test? I looked through but couldn't find the way.

OriolAbril

pystan refitting and numba notebooks have no inferencedata so there is no need to rerun them

arviz/utils.py

percygautam · 2020-06-06T11:10:13Z

Actually I did revert some commits as I have forgot to gitignore the cache of notebooks and pushed them. I'll check and make all the changes again.

codecov · 2020-06-06T12:10:31Z

Codecov Report

Merging #1217 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1217      +/-   ##
==========================================
- Coverage   93.09%   93.09%   -0.01%     
==========================================
  Files          95       94       -1     
  Lines        9591     9525      -66     
==========================================
- Hits         8929     8867      -62     
+ Misses        662      658       -4

Impacted Files	Coverage Δ
arviz/data/inference_data.py	`80.11% <100.00%> (+0.71%)`	⬆️
arviz/utils.py	`89.56% <100.00%> (+0.23%)`	⬆️
arviz/data/__init__.py	`100.00% <0.00%> (ø)`
arviz/data/io_pyjags.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8b14cf...696b8a4. Read the comment docs.

arviz/tests/base_tests/test_data.py

canyon289

Still needs more tests, Please add them. Approving so I don't block you

canyon289 · 2020-06-06T14:02:43Z

arviz/data/inference_data.py

 msg = "Inference data with groups:\n\t> {options}".format(
 options="\n\t> ".join(self._groups)
 )
 if self._groups_warmup:
 msg += "\n\nWarmup iterations saved ({}*).".format(WARMUP_TAG)
 return msg

+ def _repr_html_(self):
+ """Make html representation of InferenceData object."""
+ display_style = OPTIONS["display_style"]


I am going to request tests before merge. IMO all code should be tested if it can, and this is no exception :). I would add a test that calls _repr_html_ directly and tests that the html string is returned.
I would also add a tests the correct repr is returned without an explicit call. I assume this will require some mocking and to "trick" the non jupyter notebook test to provide the html repr.

I have added the basic test. How do I add the second test? I looked through but couldn't find the way.

This is going to be fun one. Making tests is sometimes a puzzle and harder than the actual programming itself, but important if you want you feature to be stable and not broken by any others.

In the test you'll have to monkeypatch OPTIONS to so when the test executes the value is text to test that this if condition works. Just being up front here it's going to be hard and confusing, but youre going to learn a lot more about python.

I don't want to block this whole PR for that one thing though. We can merge but I really would like you to add this test in a followup PR. At a minimum create an issue so we can track

https://docs.pytest.org/en/latest/monkeypatch.html

I think it will not be necessary to monekypatch because the display style option is already customizable similarly to an rcParam. We can start the test by setting the display_style to text/html, make sure the correct repr is returned and then finish the test by going back to the original displaystyle (much like your original implementation of the repr @percygautam).

Its typically bad practice to have tests rely on global state like rcparam although in this case lets just do it cause monkeypatching would be annoying :D

I am okay with both options. We can currently implement the test using rcParams, and create the issue to track the monkeypatch test and merge this PR. I will implement the test later.
The html_repr really facilitates to check other methods that I am adding, so it'll be better to merge this one as soon as possible.

Agree with merging as soon as possible! Seems like youre only a couple linting errors away :)

arviz/tests/base_tests/test_data.py

canyon289 · 2020-06-09T03:40:43Z

Add changelog and this one should be good to go

OriolAbril · 2020-06-09T17:22:49Z

@percygautam can you rebase and rerun notebook? from_pyjags has just been added so it is one more case for which to show the new repr

percygautam added 2 commits June 1, 2020 17:28

added html representation for jupyter notebooks

abd6430

black change

8b3caf8

canyon289 reviewed Jun 1, 2020

View reviewed changes

arviz/data/inference_data.py Outdated Show resolved Hide resolved

canyon289 reviewed Jun 1, 2020

View reviewed changes

arviz/data/inference_data.py Outdated Show resolved Hide resolved

canyon289 reviewed Jun 1, 2020

View reviewed changes

arviz/utils.py Outdated Show resolved Hide resolved

canyon289 requested changes Jun 1, 2020

View reviewed changes

OriolAbril reviewed Jun 1, 2020

View reviewed changes

arviz/data/inference_data.py Outdated Show resolved Hide resolved

arviz/data/inference_data.py Outdated Show resolved Hide resolved

arviz/utils.py Outdated Show resolved Hide resolved

percygautam added 4 commits June 2, 2020 01:30

used xarray's default and changed formatting

13cb79c

pylint changes

22bb29f

used PascalStyle names

c30a54e

used PascalStyle names

e670a8d

OriolAbril reviewed Jun 2, 2020

View reviewed changes

styling changes and pydocstyle corrections

350cf5e

canyon289 reviewed Jun 3, 2020

View reviewed changes

aloctavodia approved these changes Jun 3, 2020

View reviewed changes

canyon289 reviewed Jun 3, 2020

View reviewed changes

arviz/data/inference_data.py Outdated Show resolved Hide resolved

canyon289 reviewed Jun 3, 2020

View reviewed changes

arviz/utils.py Outdated Show resolved Hide resolved

canyon289 reviewed Jun 3, 2020

View reviewed changes

arviz/utils.py Show resolved Hide resolved

percygautam added 7 commits June 3, 2020 23:31

updated the cookbook notebook

567b82c

updated the other notebooks

391fe27

minor changes

f55d99a

added test hyml repr

b18ab34

revert last commit

dd43b60

revert last commit

e891236

revert last commit

93c3dcd

deleted unwanted files

a283a2e

percygautam added 4 commits June 5, 2020 01:34

rerun the notebooks

64862b2

pylint correction

bfc0523

pydocstyle correction

f0e8d8d

again pydocstyle correction

981f640

OriolAbril mentioned this pull request Jun 5, 2020

Added support for PyJAGS in ArviZ #1219

Merged

6 tasks

black correction

f955c4f

OriolAbril reviewed Jun 5, 2020

View reviewed changes

arviz/utils.py Outdated Show resolved Hide resolved

percygautam added 2 commits June 6, 2020 16:59

corrrected problems from reverted commits

e665934

pylint correction

940a956

canyon289 reviewed Jun 6, 2020

View reviewed changes

arviz/tests/base_tests/test_data.py Show resolved Hide resolved

canyon289 reviewed Jun 6, 2020

View reviewed changes

arviz/tests/base_tests/test_data.py Show resolved Hide resolved

canyon289 approved these changes Jun 6, 2020

View reviewed changes

canyon289 reviewed Jun 6, 2020

View reviewed changes

percygautam added 2 commits June 6, 2020 23:42

group_id correction and rerun the notebooks

a2035e7

add docstring to test

d4a49cd

OriolAbril reviewed Jun 6, 2020

View reviewed changes

arviz/tests/base_tests/test_data.py Show resolved Hide resolved

percygautam added 3 commits June 7, 2020 00:35

pylint solved

850fd23

added more tests

550f1bc

minor nits

c08f288

percygautam added 2 commits June 9, 2020 14:12

add change to changelog

30ecaab

minor fix

bb75e3b

resolve conflict

696b8a4

OriolAbril merged commit 661e057 into arviz-devs:master Jun 9, 2020

percygautam mentioned this pull request Jun 10, 2020

Re-run the notebooks to add PyJAGS #1234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

html representation for jupyter notebooks #1217

html representation for jupyter notebooks #1217

percygautam commented Jun 1, 2020 •

edited

Loading

canyon289 left a comment •

edited

Loading

OriolAbril left a comment

percygautam commented Jun 1, 2020 •

edited

Loading

OriolAbril Jun 2, 2020

OriolAbril Jun 6, 2020

canyon289 left a comment

OriolAbril commented Jun 3, 2020

canyon289 commented Jun 3, 2020 •

edited

Loading

canyon289 Jun 3, 2020

canyon289 commented Jun 3, 2020

percygautam commented Jun 4, 2020

OriolAbril left a comment

percygautam commented Jun 6, 2020

codecov bot commented Jun 6, 2020 •

edited

Loading

canyon289 left a comment

canyon289 Jun 6, 2020

OriolAbril Jun 6, 2020

canyon289 Jun 7, 2020

percygautam Jun 7, 2020

canyon289 Jun 8, 2020

canyon289 commented Jun 9, 2020

OriolAbril commented Jun 9, 2020

html representation for jupyter notebooks #1217

html representation for jupyter notebooks #1217

Conversation

percygautam commented Jun 1, 2020 • edited Loading

Description

Checklist

canyon289 left a comment • edited Loading

Choose a reason for hiding this comment

OriolAbril left a comment

Choose a reason for hiding this comment

percygautam commented Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canyon289 left a comment

Choose a reason for hiding this comment

OriolAbril commented Jun 3, 2020

canyon289 commented Jun 3, 2020 • edited Loading

Choose a reason for hiding this comment

canyon289 commented Jun 3, 2020

percygautam commented Jun 4, 2020

OriolAbril left a comment

Choose a reason for hiding this comment

percygautam commented Jun 6, 2020

codecov bot commented Jun 6, 2020 • edited Loading

Codecov Report

canyon289 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canyon289 commented Jun 9, 2020

OriolAbril commented Jun 9, 2020

percygautam commented Jun 1, 2020 •

edited

Loading

canyon289 left a comment •

edited

Loading

percygautam commented Jun 1, 2020 •

edited

Loading

canyon289 commented Jun 3, 2020 •

edited

Loading

codecov bot commented Jun 6, 2020 •

edited

Loading