Skip to content
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

Add num and round parameters to Float sliders #2719

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

davidbrochart
Copy link
Member

Added a num parameter to FloatSlider and FloatRangeSlider, which specifies the number of values in the [min, max] interval, and infers step. It can be more intuitive than specifying step.
The num parameter takes precedence over the step parameter.
See the step API as the equivalent of numpy.arange, and the num API as the equivalent of numpy.linspace.
Fixes #2287 and such issues where one specifies a max value which is not included although max = min + n * step. With the num API, max is always included.

Added a round parameter to FloatSlider, which if True rounds the value set from Python to the closest value in the set of possible values (according to step or num), and if False raises an exception if the specified value is not in the set of possible values (default value is True).

@davidbrochart
Copy link
Member Author

Default value of round is now False, and no error is raised when a value set from the Python side doesn't fall in the set of possible values (this is identical to the previous behavior).
Ready to be reviewed/merged.

@marimeireles
Copy link
Member

Hey @davidbrochart so the way this was supposed to work is, in an example like this:

import ipywidgets as wd
A = wd.FloatSlider(value=0.60, min=0.4, max=0.70, step=0.05, round=True)
A

The slider should go all the way to 70?

@davidbrochart
Copy link
Member Author

No, this PR introduces a new num parameter. You should use either step or num (num takes precedence over step). num sets the number of values in the [min, max] interval, and ensures that the max value is always included (which might not be the case due to floating point precision issues if the step parameter is used).
In your example, the slider should go all the way to 0.7 if you use num=6 instead of step=0.05.

@marimeireles
Copy link
Member

Ah! I see, so it works for me :)
And I was wondering, the FloatLogSlider class doesn't need to receive something like this?

@davidbrochart
Copy link
Member Author

Good point, maybe yes. And also FloatRangeSlider and FloatProgress?
What about the Int equivalents? If yes, should we ensure that the step inferred from num is a integer value? Otherwise the max value won't be included.

@marimeireles
Copy link
Member

Your changes also work rebasing on the big PR.

@davidbrochart
Copy link
Member Author

I don't think we should add a num parameter to Int sliders, because the purpose of this parameter is to ensure there's going to be num values, and this might not be possible with e.g. num=3 in the interval [0, 1]. And round is useless for Int sliders of course.
@marimeireles do you want to give it a final review?

@ianhi
Copy link
Contributor

ianhi commented Feb 9, 2021

The num parameter takes precedence over the step parameter.

Given this perhaps the way interact generates sliders should also be changed? That is a far more disruptive change unfortunately. But if num takes precedence over step then I would expect interact(f, param=(0, 10, 5)) to treat the 5 as a num rather than step

@davidbrochart
Copy link
Member Author

I'm not sure to follow you. What I meant is that if you pass num, then it "overwrites" step, since it actually infers it. But we can keep the same behavior for interact as before, with step as the "default" parameter.

@marimeireles
Copy link
Member

Sorry for the delay @davidbrochart. All works as expected for me :)
What is this read the docs error? Should we worry about it (talking about a 8.0 release scope)

@davidbrochart
Copy link
Member Author

Yeah I'm not sure about the RTD issue, it doesn't seem to be related to this PR.

@ianhi
Copy link
Contributor

ianhi commented Feb 9, 2021

Yeah I'm not sure about the RTD issue, it doesn't seem to be related to this PR

If you rebase that error will be fixed. See #3098

I'm not sure to follow you. What I meant is that if you pass num, then it "overwrites" step, since it actually infers it. But we can keep the same behavior for interact as before, with step as the "default" parameter.

Sorry, all I meant was that with this the the default behavior will be slightly divergegent for these two different ways of creating sliders. For direct creation num will be preferred, but interact with a tuple is implicitly saying that step takes precedence. This isn't a big deal, just pointing it out. Switching the default to num is an improvement regardless.

@davidbrochart
Copy link
Member Author

davidbrochart commented Feb 9, 2021

Ah yes you're right, num is kind of the default now. This is because it infers step, and I can't change this default behavior because step has a valid default value, so I can't check if it's None for instance.
And as you said changing interact's API would be too disruptive, so I guess it's better to keep it like that.
I just rebased on master, thanks @ianhi !

@davidbrochart
Copy link
Member Author

I realized the _validate_value that I added to optionally round values was shadowing the one in the base class (_BoundedFloat will clip the value in [min, max]).

@jasongrout jasongrout modified the milestones: 8.0, 8.1 Jun 1, 2021
@jasongrout
Copy link
Member

Putting this as 8.1 since it is additive and not backwards incompatible. If this is finished before the 8.0 beta, then it can certainly go in!

@davidbrochart
Copy link
Member Author

@jasongrout this is ready to go in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FloatSlider maximum not inclusive?
4 participants