-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Matplotlib now supports custom projections as plot option #510
Conversation
projs = [p for p in projs if p is not None] | ||
projs = self._deep_options(view, 'plot', ['projection'], | ||
[CompositeOverlay])['projection'] | ||
if projs: |
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.
Little confused...is this supposed to be if not projs
? Seems you would try to look for the options on the elements if they couldn't be found at the overlay level, especially since we only allow one projection anyway...
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.
You're right.
There are now conflicts but I was happy to merge once you had checked that the lbrt logic was correct... |
I'll resolve the conflict soon. For now this solution is fine but I may open an issue to make some of the methods on Plot into utility functions. |
@@ -82,8 +82,7 @@ class MPLPlot(DimensionedPlot): | |||
sublabel_size = param.Number(default=18, doc=""" | |||
Size of optional subfigure label.""") | |||
|
|||
projection = param.ObjectSelector(default=None, | |||
objects=['3d', 'polar', None], doc=""" | |||
projection = param.Parameter(default=None, doc=""" |
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.
Do you validate the possible strings? I've not read the code below but we should make sure if a string is supplied it is validate...
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.
Oh and I would like some information about what a projection specification is if it isn't None or one of the strings...
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.
Doesn't validate yet but could do, although some matplotlib extensions allow new projection types to be specified as strings.
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.
To be honest creating the axes is one of the first things that happens and the traceback matplotlib provides is reasonable:
ValueError [Call ipython.show_traceback() for details]
Unknown projection 'Test'
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.
Yes, that is very reasonable.
I've made a few more comments but once those are addressed, I think this PR looks good. |
Thanks for improving the docstrings! At the |
Yes, plotly has this concept, I think there it only provides control over cartesian or polar axes and 3d plots are a separate concept but we can smooth over that inconsistency with our API. I do think it's a general concept and the reason why it's a parameter at the Plot level is because some of general extents finding code has been written to handle 3d axes. In future hopefully Bokeh will also support custom projections including various geographic projections, polar plots and maybe 3d plots as well. |
Ready to merge now. I'll open a new issue to discuss which bits on Plot can become utilities. |
Looks good! |
Matplotlib now supports custom projections as plot option
This PR allows custom projections to be specified on an Element. Also includes a number of fixes to ensure that extents are computed correctly in those cases.