-
Notifications
You must be signed in to change notification settings - Fork 235
Make IPython partially optional on CI to increase test coverage of figure.py #1496
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
Merged
Merged
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
4c88e0d
Install IPython as an optional dependency only for Python 3.9 CI tests
weiji14 e5fddaa
Add pytest.importorskip ipython to test_figure_show
weiji14 ef11d94
Add pytest.importorskip ipython and sphinx_gallery to test_pygmtscrapper
weiji14 32e5483
Skip running the fig.inset doctest's fig.show line
weiji14 7273282
Merge branch 'main' into optional-ipython-tests
weiji14 8512189
Add test for fig.show using notebook method when IPython isn't installed
weiji14 939171c
Add tests for figure's PNG and HTML printable representations
weiji14 de6727a
Provide reason for skipping test_figure_show and test_pygmtscrapper
weiji14 59ba1c9
Silence pylint protected-access warning on accessing PNG and HTML repr
weiji14 cae6190
Try test figure display external method
weiji14 edd1aa8
Merge branch 'main' into optional-ipython-tests
weiji14 8ba65ea
Typo fix
weiji14 a9a5796
Move pytest.importorskip IPython outside of test_pygmtscrapper
weiji14 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is
_repr_png_and_repr_html_useful? I have no idea how these functions can be used.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.
Good question. They are used by
IPython.display.display_pngandIPython.display.display_htmlrespectively, i.e. for display in Jupyter notebooks.Technically I should have written these unit test using
IPython.display.display_png(fig)andIPython.display.display_html(fig)instead of testing these non-public functions directly, but it's hard to test a pop-up figure. So I've settled for testing the__repr_png__and__repr_html__instead (which works with or without IPython installed). Con with this approach is that I need to dopylint: disable=protected-access🙃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.
PyGMT already provides the
Figure.show()function for display in Jupyter notebooks. Removing_repr_png_()and_repr_html_()doesn't affect theFigure.show()function. Do you think we should remove these two functions?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.
It might not affect
Figure.show(), but I think we need to keep it. Having__repr_png__()and__repr_html__means that putting justfigat the last line of a Jupyter notebook cell will display the plot. E.g. like this:This works in Jupyter/IPython because of a
IPython.display.display_html(fig)call. Similarly, if you put adf(pandas.DataFrame) on the last line of a cell, a nicely formatted html table will be printed because Jupyter/IPython doesIPython.display.display_html(df)(see the__repr_html__at https://github.com/pandas-dev/pandas/blob/73c68257545b5f8530b7044f56647bd2db92e2ba/pandas/core/frame.py#L1007-L1049).