Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Update Slider docs and type check slidermin and slidermax. #8134
Conversation
heath730
added some commits
Feb 23, 2017
| + Notes | ||
| + ---------- |
|
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. |
tacaswell
added this to the
2.1 (next point release)
milestone
Feb 23, 2017
|
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. |
| Do not allow the current slider to have a value less than | ||
| - `slidermin` | ||
| + `slidermin.val`. |
WeatherGod
Feb 23, 2017
Member
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 ;) ). |
| + dragging : bool, optional | ||
| + If True the slider can be dragged by the mouse. | ||
| + Default: True |
| + ----- | ||
| + 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 |
WeatherGod
Feb 23, 2017
Member
If you put a tilda before the matplotlib.patches.Rectangle, then the rendered docs will only have Rectangle, which makes things easier to read.
| + 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*, ...). |
| + if not self.closedmax: | ||
| + return | ||
| + val = self.slidermax.val | ||
| + self.set_val(val) |
WeatherGod
Feb 23, 2017
Member
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.
NelleV
reviewed
Feb 23, 2017
Apart from my minor comments on docstring formatting, this looks good.
If you prefer, I am happy fixing those just before merging.
| - less than *slidermax* | ||
| - | ||
| - *dragging* : allow for mouse dragging on slider | ||
| + Create a slider from *valmin* to *valmax* in axes *ax*. For the slider to |
| - The slider label | ||
| + valinit : float, optional | ||
| + The slider initial position. | ||
| + Default: 0.5 |
NelleV
Feb 23, 2017
Contributor
Can you add the default on the line that defines the parameter?
(note that it is currently not indented properly).
| - Used to format the slider value, fprint format string | ||
| + valfmt : str, optional | ||
| + Used to format the slider value, fprint format string. | ||
| + Default: '%1.2f' |
NelleV
Feb 23, 2017
Contributor
Same (Can you add the default on the line that defines the parameter?)
| - 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 |
NelleV
Feb 23, 2017
Contributor
Same (Can you add the default on the line that defines the parameter?)
| - 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 |
NelleV
Feb 23, 2017
Contributor
Same (Can you add the default on the line that defines the parameter?)
| Do not allow the current slider to have a value less than | ||
| - `slidermin` | ||
| + the value of the Slider `slidermin`. | ||
| + Default: None |
NelleV
Feb 23, 2017
Contributor
Same (Can you add the default on the line that defines the parameter?)
| - `slidermax` | ||
| - | ||
| + the value of the Slider `slidermax`. | ||
| + Default: None |
NelleV
Feb 23, 2017
Contributor
Same (Can you add the default on the line that defines the parameter?)
| - if the slider can be dragged by the mouse | ||
| + dragging : bool, optional | ||
| + If True the slider can be dragged by the mouse. | ||
| + Default: True |
NelleV
Feb 23, 2017
Contributor
Same (Can you add the default on the line that defines the parameter?)
| + 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. |
NelleV
approved these changes
Feb 24, 2017
I pushed some minor nitpicks. It looks good to me.
thanks a lot for the work on this PR @heath730 !
NelleV
changed the title from
Update Slider docs and type check slidermin and slidermax. to [MRG+2] Update Slider docs and type check slidermin and slidermax.
Feb 24, 2017
|
Thanks! Going squash a couple of the doc commits and push once more. |
heath730
and others
added some commits
Feb 23, 2017
|
heath730 |
9d3e5e1
|
|||
|
|
heath730 |
ea899aa
|
|||
|
|
heath730 |
dc55f80
|
|||
|
|
NelleV |
Some checks were not successful
continuous-integration/travis-ci/pr
— The Travis CI build could not complete due to an error
codecov/project/library
— Absolute coverage decreased by -2.49% but relative coverage increased by +12.32% compared to ba418fd
|
325789a
|
||
|
|
heath730 |
All checks have passed |
2b42285
|
NelleV
merged commit c3e65f1
into matplotlib:master
Feb 25, 2017
5 checks passed
|
Thanks @heath730 ! |
heath730 commentedFeb 23, 2017
•
edited
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.