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
test: matplotlib backend side-effects #972
base: develop
Are you sure you want to change the base?
Conversation
@sbrugman clean start for tests. let me know if it needs anything else to be approved. |
] | ||
} | ||
], | ||
"metadata": { |
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.
Please remove the metadata (not relevant for the test)
Suggestion:
pip install nbconvert
jupyter nbconvert --ClearOutputPreprocessor.enabled=True --ClearMetadataPreprocessor.enabled=True --ClearMetadataPreprocessor.preserve_cell_metadata_mask='[("tags")]' --inplace notebook_plotting.ipynb
Update: added a hook for this in #973
], | ||
"source": [ | ||
"from IPython.utils import capture\n", | ||
"from IPython.display import Image, display\n", |
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 there a particular reason to import here and in the first cell?
"import pytest\n", | ||
"\n", | ||
"from pandas_profiling import ProfileReport\n", | ||
"from pathlib import Path\n", |
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.
Please remove unused imports
Thanks @jodom961. The comments above provide some pointers to improve the PR. |
"assert len(out_matplot.outputs[0].data[\"image/png\"]) > 0 #assert an image actually exists\n" | ||
] | ||
}, | ||
{ |
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.
Output image could be removed, correct?
41d376f
to
5f17cfd
Compare
a4724cf
to
47d3aeb
Compare
45b1d80
to
a0dddb9
Compare
e786ac6
to
2d55f32
Compare
6ba2217
to
ef023c3
Compare
4500563
to
cfb020d
Compare
As described in the issues #888 and #837 , upon the release of 3.1.0, a side effect of importing pandas_profiling was that it overrode the existing backend of matplotlib on import, and prevented plots from being rendered in Jupyter environments.
Since it seems the backend does not needed to be set per matplots documentation,
By default, Matplotlib should automatically select a default backend which allows both interactive work and plotting from scripts, with output to the screen and/or to a file, so at least initially you will not need to worry about the backend.
Using matplot.libuse() will require changes in your code if users want to use a different backend. Therefore, you should avoid explicitly calling use unless absolutely necessary.
and it is generally a best practice to not have side effects upon importing a package, I have elected to remove that line.
A new test is in place in notebooks/notebook_plotting.ipynb to prevent this in the future.