-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Rename parameter selectors #20585
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
Rename parameter selectors #20585
Conversation
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.
Seems reasonable, left 2 small comments.
Thanks for the review @tacaswell, it should be good now. |
Should be all done! |
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 know that some of the commented changes have precedence in other widget classes, however I'm not convinced, we should go that direction (e.g. code blocks for defaults).
lib/matplotlib/widgets.py
Outdated
|
||
dict(color='k', linestyle='-', linewidth=2, alpha=0.5) | ||
|
||
handle_props : dict |
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.
Let's the name markerprops
. This is a bit different from SpanSelector
. First, here we have lineprops
+ markerprops
, keeping this consistent within the class is more important than consistency with another class. Second, while semantically we have handles in both classes, they are realized by markers and lines respectively and thus accept different parameters. I favor making this clear from the name.
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 see what you what you mean, but I am convinced that simplifying the API and making it more generic is worth it at the cost of the being slightly less precise. I agree that using a more specific terminology is more explicit but at the end of the day, when we create a selector - regardless of what selector it is - what we want is to set the properties of the patch and/or the handle, regardless of what their are or of their implementation details.
For example, as an average matplotlib user, I don't know what are the different parameters between lineprops
and markerprops
, I will need to look it up.
In 35ee92d, I have changed the various patch properties (rectprops
, lineprops
) argument to props
, so that now we have only props
and handleprops
for all selectors.
To address #20618, we could add two method to set the properties of the selector patch and the handle separately. The idea is that either at initialisation or when using set_props
or set_handle_props
, we could set the properties regardless of what type of object they are.
I am happy to revert 35ee92d if others also think that this is going in the wrong direction!
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.
For example, as an average matplotlib user, I don't know what are the different parameters between
lineprops
andmarkerprops
, I will need to look it up.
Conversely, I argue that props
is even less helpful. The different selectors are realized by different Artists and thus have different properties. We cannot hide that fact. SpanSelector
accepts facecolor
, LassoSelector
accepts solid_linestyle
, and neither accepts the other one respectively. With props
the user has to lookup what is supported as well. But it's more surpising and harder to remember that the same parameter name accepts different inputs depending on the class.
lib/matplotlib/widgets.py
Outdated
state_modifier_keys : dict, optional | ||
Keyboard modifiers which affect the widget's behavior. Values | ||
amend the defaults. | ||
@docstring.Substitution(_RECTANGLESELECTOR_PARAMETERS_DOCSTRING) |
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.
Why this indirection?
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 able to reuse the docstring in EllipseSelector and RectangleSelector.
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 see. I'm not a fan of too much indirection in the docs. Some duplication is ok sometimes.
Reuse implies coupling. The risk with indirection is that people start to write about Rectangle explicitly, which does not make sense for the Ellipse case. Conversly, the risk with duplicated docstrings is that they are not in sync (which IMHO is not the end of the world).
I even claim it's a mistake that EllipseSelector
inherits from RectangleSelector
.
It's a tradeoff which I would decide in favor of the duplication here. But I'm not going to fight over it.
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 have put a placeholder for the artist type to be replaced by ellipse
or selector
to fix this issue.
I even claim it's a mistake that
EllipseSelector
inherits fromRectangleSelector
.
I don't know about that, maybe for another PR! ;)
Ok, so here is an overview of the current APIs
Given the overall situation I propose the following:
|
97456b4
to
a22f8cb
Compare
We should not worry about |
Thanks for pointing this out, I have overlooked this. However, I don't think it changes the above assessment. We're still at |
I think that I have addressed all comments. I have no idea why the doc build is failing and I can't see how it is related to this PR; the warning is:
|
A rebase should fix that. |
…ce for RectangleSelector
…`handle_grad_distance` in PolygonSelector
f8df989
to
8ff493c
Compare
Ok, thanks! |
… deprecation Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Thanks @timhoffm, I have updated the span selector example too. |
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 for the quick response. I did a careful full review now, which revealed still some open issues.
b3925a9
to
8682b5f
Compare
PR Summary
Follow up of #20558. Rename parameters (
maxdist
,markers_props
, etc.) to make them consistent between the different selectors.PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).