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

Unify and fix testing of .plot() methods #1745

Closed
adonath opened this issue Aug 24, 2018 · 2 comments
Closed

Unify and fix testing of .plot() methods #1745

adonath opened this issue Aug 24, 2018 · 2 comments
Assignees
Milestone

Comments

@adonath
Copy link
Member

adonath commented Aug 24, 2018

Currently we are testing the .plot() methods of our data classes, just by calling .plot() and in some cases by calling mpl_savefig_check() after. When multiple plot methods are executed in one test session, the plots are drawn on the same axes, because we typically call plt.gca() to get the current axes object. This leads to unexpected warnings and funny plots such as:

test

I discussed with @cdeil we should introduce a check_mpl() context manager that deletes current axes objects in __enter__ and calls the savefig check in __exit__. All places in Gammapy where we test plot methods should the be adapted accordingly.

@cdeil
Copy link
Contributor

cdeil commented Aug 24, 2018

context manager that deletes current axes objects in enter and calls the savefig check in exit

I think we should do plt.figure to make a new figure in __enter__ instead of deleting existing things, and close it in __exit__. See example and discussion in matplotlib/matplotlib#5218

All places in Gammapy where we test plot methods should the be adapted accordingly.

You can get a complete list of all existing plot test with:

$ ack requires_dependency | grep matplotlib

Of course it would be nice if you could also check somehow if we have completely untested plot methods left; this is a good start:

$ ack def | grep plot | egrep -v test

Although that also comes out of missing test coverage, so we'll get to that later anyways in a systematic fashion.

Thanks for doing this cleanup!

@cdeil cdeil added this to the 0.8 milestone Aug 24, 2018
@adonath
Copy link
Member Author

adonath commented Aug 27, 2018

Resolved by #1746.

@adonath adonath closed this as completed Aug 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants