-
Notifications
You must be signed in to change notification settings - Fork 3
Plots/Refactor balloon plot (ax, colorbar) #220
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
Conversation
sikersten
left a comment
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.
Thank you, very nice!
ahms5
left a comment
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.
Nice, thanks, just one question out the retrun of ax.
ahms5
left a comment
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.
Thanks looks good, just have a minor optional suggesstion, approved anyways.
tests/test_plots.py
Outdated
| if function.__name__ in ["balloon"]: | ||
| cb = function(coords, data, colorbar=colorbar)[2] | ||
| out = function(coords, data, colorbar=colorbar) | ||
| if colorbar: | ||
| assert isinstance(cb, mpl.colorbar.Colorbar) | ||
| assert isinstance(out[2], mpl.colorbar.Colorbar) | ||
| assert isinstance(out[0], list) | ||
| if not colorbar: | ||
| assert cb is None | ||
| assert out[2] is None | ||
| assert isinstance(out[0], plt.Axes) |
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.
very optional: could be a dedicated test for testing the outputs for ballon plots
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.
I thought about that too, but it would be very redundant to what is already tested.
Could you please elaborate what you mean by this? If this information needs to survive beyond this PR, please open an issue. |
|
To have the tests run without failing i added cases for the refactored functions with new ax and colorbar returns. e.g.: |
|
Also #214 should be merged first |
I've updated the description to include this information. |
Changes proposed in this pull request:
Discussion
The
if function.__name__ in ...cases in the tests can be removed once all plot functions are adjusted.Depends on #214