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 #1746

Merged
merged 15 commits into from Aug 27, 2018

Conversation

Projects
None yet
2 participants
@adonath
Copy link
Member

adonath commented Aug 25, 2018

This PR fixes the issue described in #1745, by introducing the mpl_plot_check() context manager and changes all the exiting plot test to use it.

@adonath adonath added this to the 0.8 milestone Aug 25, 2018

@adonath adonath self-assigned this Aug 25, 2018

@adonath adonath force-pushed the adonath:issue_#1745 branch from 9eb5a72 to 1ea82a4 Aug 25, 2018

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Aug 26, 2018

@adonath - Thanks!

We should close the figure in __exit__, no?

Suggest to remove alias mpl_plot_check = _MPLPlotCheck and directly do class mpl_plot_check.

Maybe add a section to mention this at http://docs.gammapy.org/dev/development/howto.html ?
I mean just one sentence and one code example, very short.
In my experience it's useful to have these little sections and a link to share, otherwise you keep explaining the same things again and again in code review and on Slack to new contributors.

Otherwise, 👍 to merge.

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Aug 27, 2018

@adonath - I think the DM plot method is broken; from the tutorial notebook:
https://gist.github.com/cdeil/3b74bbd2082dfa549acc4fb2f4af4d9f
Can you please delete that if cbar code or make it covered by a test?

@adonath adonath force-pushed the adonath:issue_#1745 branch from 1ea82a4 to 7d32a2b Aug 27, 2018

adonath added some commits Aug 27, 2018

@adonath adonath merged commit bb14921 into gammapy:master Aug 27, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@cdeil cdeil changed the title Unify and fix testing of .plot() methods Unify and fix testing of plot methods Sep 9, 2018

@adonath adonath deleted the adonath:issue_#1745 branch Nov 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.