Update Slider docs and type check slidermin and slidermax. #8134
Conversation
lib/matplotlib/widgets.py
Outdated
|
|
||
| Notes | ||
| ---------- |
There was a problem hiding this comment.
picky thing, too many -- here, should be same length as title.
|
While you are working on this exercising all of the branches of the validation method would be good, but not worth holding this PR up over. |
|
Thanks @tacaswell. I addressed your comments in the recent commits. Let me know if I should squash some commits and rebase on master pre-merge. |
lib/matplotlib/widgets.py
Outdated
| Do not allow the current slider to have a value less than | ||
| `slidermin` | ||
| `slidermin.val`. |
There was a problem hiding this comment.
slidermin.val? Is that some sort of special numpydoc thing?
There was a problem hiding this comment.
It is just saying it looks at the attribute I think.
There was a problem hiding this comment.
Ah, I think I see what is going on here. This needs to be clearer. Perhaps, "Do not allow the slider to have a value less than the value of the other Slider slidermin."
|
Fine as is, rebase/squash if you want (I do it most when I am embarrassed about a mistake I committed ;) ). |
lib/matplotlib/widgets.py
Outdated
|
|
||
| dragging : bool, optional | ||
| If True the slider can be dragged by the mouse. | ||
| Default: True |
There was a problem hiding this comment.
Do we have to do special formatting for things like True and None?
lib/matplotlib/widgets.py
Outdated
| ----- | ||
| Additional kwargs are passed on to ``self.poly`` which is the | ||
| :class:`matplotlib.patches.Rectangle` that draws the slider | ||
| knob. See the :class:`matplotlib.patches.Rectangle` documentation for |
There was a problem hiding this comment.
If you put a tilda before the matplotlib.patches.Rectangle, then the rendered docs will only have Rectangle, which makes things easier to read.
lib/matplotlib/widgets.py
Outdated
| Additional kwargs are passed on to ``self.poly`` which is the | ||
| :class:`matplotlib.patches.Rectangle` that draws the slider | ||
| knob. See the :class:`matplotlib.patches.Rectangle` documentation for | ||
| valid property names (e.g., *facecolor*, *edgecolor*, *alpha*, ...). |
There was a problem hiding this comment.
No need for ellipses when it is already "e.g.".
lib/matplotlib/widgets.py
Outdated
| if not self.closedmax: | ||
| return | ||
| val = self.slidermax.val | ||
| self.set_val(val) |
There was a problem hiding this comment.
I think I would rather that this method returns the value. It can then be up to the caller what to do with it. Might make it more portable with the planned traitlets work.
lib/matplotlib/widgets.py
Outdated
| less than *slidermax* | ||
|
|
||
| *dragging* : allow for mouse dragging on slider | ||
| Create a slider from *valmin* to *valmax* in axes *ax*. For the slider to |
There was a problem hiding this comment.
Can you remove the "*" formatting?
lib/matplotlib/widgets.py
Outdated
| The slider label | ||
| valinit : float, optional | ||
| The slider initial position. | ||
| Default: 0.5 |
There was a problem hiding this comment.
Can you add the default on the line that defines the parameter?
(note that it is currently not indented properly).
lib/matplotlib/widgets.py
Outdated
| Used to format the slider value, fprint format string | ||
| valfmt : str, optional | ||
| Used to format the slider value, fprint format string. | ||
| Default: '%1.2f' |
There was a problem hiding this comment.
Same (Can you add the default on the line that defines the parameter?)
lib/matplotlib/widgets.py
Outdated
| Indicate whether the slider interval is closed on the bottom | ||
| closedmin : bool, optional | ||
| Indicate whether the slider interval is closed on the bottom. | ||
| Default: True |
There was a problem hiding this comment.
Same (Can you add the default on the line that defines the parameter?)
lib/matplotlib/widgets.py
Outdated
| Indicate whether the slider interval is closed on the top | ||
| closedmax : bool, optional | ||
| Indicate whether the slider interval is closed on the top. | ||
| Default: True |
There was a problem hiding this comment.
Same (Can you add the default on the line that defines the parameter?)
lib/matplotlib/widgets.py
Outdated
| Do not allow the current slider to have a value less than | ||
| `slidermin` | ||
| the value of the Slider `slidermin`. | ||
| Default: None |
There was a problem hiding this comment.
Same (Can you add the default on the line that defines the parameter?)
lib/matplotlib/widgets.py
Outdated
| `slidermax` | ||
|
|
||
| the value of the Slider `slidermax`. | ||
| Default: None |
There was a problem hiding this comment.
Same (Can you add the default on the line that defines the parameter?)
lib/matplotlib/widgets.py
Outdated
| if the slider can be dragged by the mouse | ||
| dragging : bool, optional | ||
| If True the slider can be dragged by the mouse. | ||
| Default: True |
There was a problem hiding this comment.
Same (Can you add the default on the line that defines the parameter?)
lib/matplotlib/widgets.py
Outdated
| Additional kwargs are passed on to ``self.poly`` which is the | ||
| :class:`~matplotlib.patches.Rectangle` that draws the slider | ||
| knob. See the :class:`~matplotlib.patches.Rectangle` documentation for | ||
| valid property names (e.g., *facecolor*, *edgecolor*, *alpha*). |
| self.valmin = valmin | ||
| self.valmax = valmax | ||
| valinit = self._value_in_bounds(valinit) |
|
Thanks @NelleV, I made the changes you suggested where it was possible to fit the default listing on the same line. Let me know if there is anything else, I'd be happy to squash this down for you guys before merging especially all the nits/lints. |
|
Thanks! Going squash a couple of the doc commits and push once more. |
9ac31fe to
325789a
Compare
|
Thanks @heath730 ! |
This addresses #7687.
I tried to improve the doc strings of the widgets.slider class using numpydoc format. Let me know if it should be handled differently or I missed something. I built the docs and it rendered cleaner than previously using the current version.
Raises a ValueError if the slidermin or slidermax objects are not a compatible type (None or hasattr( 'val')).
Adds a test to make sure exception is raised in appropriate cases.
As I was testing this I noticed that the initial value can be outside of the bounds, and it will not be updated until _update gets called. To correct this I wrapped the statements that ensure the value is within the provided bounds into a function and called it in the the constructor. Currently there is no validation on the bounds, but I would be willing to add that if you think it will help.