-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
Let as_mpl_selector
forward **kwargs to *Selector
#392
Conversation
9844800
to
c665b84
Compare
Yes, let's mention |
5b061a3
to
8eee8c8
Compare
Marking this as ready for review now, although there could be one or two tests for passing other kwargs added, + the docstring for |
2d69768
to
4a1c1fc
Compare
I should note that the second part of Also not sure if the version check is worth importing Lastly, the |
I've pulled the Matplotlib checks up one level into I've also included a fix to the Mpl intersphinx setup for testing: it seems the base URL has been moved around 3.5 from https://matplotlib.org/ to https://matplotlib.org/stable/ ; if you compare the link to |
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.
lgtm; I haven't run this locally but it looks good.
9256a30
to
e0e6095
Compare
Changed to |
Is this ready for a final review? |
e0e6095
to
85fdffd
Compare
Yes, should now - there was a call from the |
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, @dhomeier! This looks great. I had just a minor comment about checking the matplotlib version. I'll be glad to stop getting emails about the failing devdeps
CI test!
Tests are now failing after the updated |
@dhomeier There are merge conflicts. Can you please rebase? |
10551e0
to
c862fa4
Compare
Doh, was caught right in between by 62eb153 (from the test I thought |
Thanks! |
The docstring for the
as_mpl_selector
methods states that any additional**kwargs
can be passed to the respective matplotlib selector instances, but currently they are in fact discarded. This PR passes them through.In addition I have added the option to override the default
rectprops
by kwarg – which otherwise might conflict with the argument specified by the user, although it's perhaps to be discussed whether these should always be set byregion.visual
.And I think there is a point to have some parameters still set to different defaults from mpl, if not explicitly set by the user, e.g.
{'edgecolor': 'black', 'facecolor': 'none'}
is certainly the more reasonable default here.Also todo tests (?); should any/which additional parameters, like
drag_from_anywhere
, be specifically mentioned in the docstring?