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

Added FloatLogSlider #1928

Merged
merged 2 commits into from Mar 23, 2018

Conversation

@sebasguts
Copy link
Contributor

commented Jan 23, 2018

a slider that uses a logarithmic scale. Closes #719

@sebasguts sebasguts force-pushed the sebasguts:master branch from ef80229 to 9102aca Jan 23, 2018

@jasongrout jasongrout added this to the Minor release milestone Feb 16, 2018

@jasongrout
Copy link
Member

left a comment

Thanks again for adding this! There are a few places I noted where it's confusing what is talking about the exponent and what is talking about the actual value. Can you look at those?

@@ -58,6 +58,39 @@ def _validate_max(self, proposal):
self.value = max
return max

class _BoundedLogFloat(_Float):
max = CFloat(10.0, help="Max value").tag(sync=True)

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 19, 2018

Member

Can we clarify that the min/max are exponents, while the value is an actual number? That was confusing to me.


@validate('max')
def _validate_max(self, proposal):
"""Enforce min <= value <= max"""

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 19, 2018

Member

We should compare value to the base**min and base**max, right?

min = proposal['value']
if min > self.max:
raise TraitError('Setting min > max')
if min > self.value:

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 19, 2018

Member

We should compare value to the base**min and base**max, right?

@@ -141,6 +176,46 @@ class FloatSlider(_BoundedFloat):
continuous_update = Bool(True, help="Update the value of the widget as the user is holding the slider.").tag(sync=True)
disabled = Bool(False, help="Enable or disable user changes").tag(sync=True)

style = InstanceDict(SliderStyle).tag(sync=True, **widget_serialization)

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 19, 2018

Member

Thanks for catching that we didn't have the right style!

value : float
position of the slider
min : float
minimal position of the slider

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 19, 2018

Member

Again, please clarify min/max are exponents

minimal position of the slider
max : float
maximal position of the slider
step : float

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 19, 2018

Member

Please clarify step is a step in the exponent, not the actual value.

"""
_view_name = Unicode('FloatLogSliderView').tag(sync=True)
_model_name = Unicode('FloatLogSliderModel').tag(sync=True)
step = CFloat(0.1, help="Minimum step to increment the value").tag(sync=True)

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 19, 2018

Member

help should clarify this is changing the exponent.

`description` | string | `''` | Description of the control.
`disabled` | boolean | `false` | Enable or disable user changes
`layout` | reference to Layout widget | reference to new instance |
`max` | number (float) | `10.0` | Max value

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 19, 2018

Member

Changing the help above to clarify min/max/step are talking about the exponent, then regenerating this, should change the docs here too.

@sebasguts sebasguts force-pushed the sebasguts:master branch 2 times, most recently from a794665 to c1c6e59 Mar 20, 2018

@sebasguts

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2018

Thanks for the review :).

I changed all the comments as proposed, and updated the PR.

return _.extend(super.defaults(), {
_model_name: 'FloatLogSliderModel',
_view_name: 'FloatLogSliderView',
step: 1.0,

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 20, 2018

Member

step has a different default than the python side.

if (isNaN(value as number)) {
this.readout.textContent = this.valueToString(this.model.get('value'));
} else {
value = Math.max(Math.min(value as number, vmax), vmin);

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 20, 2018

Member

This is comparing the actual value to vmin and vmax, which are exponents. This means that setting the value using the readout is clipped by the exponents, not the actual value range (e.g., try setting the readout to 20 with the default parameters...)

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 21, 2018

Member

Also, you don't need the as number here either.

this.touch();
}

_validate_slide_value(x) {

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 20, 2018

Member

Please dedent this line.

'.2f', help="Format for the readout").tag(sync=True)
continuous_update = Bool(True, help="Update the value of the widget as the user is holding the slider.").tag(sync=True)
disabled = Bool(False, help="Enable or disable user changes").tag(sync=True)
base = CFloat(2., help="Base for the logarithm").tag(sync=True)

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 20, 2018

Member

I know we had this discussion before - what do you think about defaulting to base 10? I think that might be more common in everyday use.

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 21, 2018

Member

If we did change it to base 10 (I think we probably ought to), we probably ought to default the max to something like 4, so we only go up to 10k.

let vmin = this.model.get('min');
let vmax = this.model.get('max');

if (isNaN(value as number)) {

This comment has been minimized.

Copy link
@jasongrout

jasongrout Mar 21, 2018

Member

You don't need the as number since the stringToValue function above is declared as returning a number.

@jasongrout

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

Thanks! Just a few more comments above, and then I think we are good to go.

@jasongrout jasongrout modified the milestones: Minor release, 7.2 Mar 21, 2018

@sebasguts sebasguts force-pushed the sebasguts:master branch from c1c6e59 to 3f4f2a6 Mar 21, 2018

@sebasguts

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2018

Thanks for the review again :). I adressed all the comments, so I also changed the base to 10 and the default max to 4.

@jasongrout

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

Thanks! I noticed one last issue in doing final testing:

screen shot 2018-03-21 at 3 38 33 pm

Just invoking with FloatLogSlider() (or FloatLogSlider(value=0)) doesn't check against the min, apparently, so we get an invalid value (the value is actually 0, it's not just a readout problem).

@jasongrout

This comment has been minimized.

Copy link
Member

commented Mar 22, 2018

I think the easiest way to correct it is to declare the default value for a _BoundedLogFloat to be one:

    value = CFloat(1.0, help="Float value").tag(sync=True)

(Would need to change the js side default too)

Added FloatLogSlider
a slider that uses a logarithmic scale. Closes #719

@sebasguts sebasguts force-pushed the sebasguts:master branch from 3f4f2a6 to 73b8132 Mar 22, 2018

@sebasguts

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2018

Thank you for the review again, and thanks for the catch. I actually did not test it without parameters.

I added the default to both sides, and the bug seems to have disappeared.

This one was quite strange, as it only seem to be appeared if value=0 was passed, value=-1 for example would have set the initial value to 1. I assume it comes from the logarithm computation on the JavaScript side, but I am not completely sure.

@jasongrout

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

Looks good. Thanks!

@jasongrout jasongrout merged commit d1997c5 into jupyter-widgets:master Mar 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sebasguts

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2018

Thanks for all the useful comments and for merging this PR :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.