fix spinner precision #13078

msalem99 · 2023-04-10T19:58:59Z

issues: fixes [BUG] Spinner in float mode with int step will increment to int #13064
tests added / passed
release document entry (if new feature or API change)

It seems that when the precision of the value is larger than the precision of the step ex value=1.333 step=1.22, the value will be truncated when incremented or decremented due to the precision of the step applying, I proceeded to save the initial value precision and took that value into consideration when calculating the precision of the incremented/decremented value.

tests have been added that test in the case of the precision of the value is larger than the step or vice versa

Please provide any feedback if things should be done differently. I am aware that setting the initial_precision_value in the render may not be best, can this be done in a better way?

codecov · 2023-04-10T20:13:38Z

Codecov Report

Merging #13078 (bf45eb3) into branch-3.4 (d620157) will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           branch-3.4   #13078   +/-   ##
===========================================
  Coverage       92.62%   92.62%           
===========================================
  Files             321      321           
  Lines           20383    20383           
===========================================
  Hits            18880    18880           
  Misses           1503     1503

bryevdv · 2023-04-10T20:46:34Z

Hi @msalem99 quick note: the selenium tests under tests/integration are currently disabled due to extreme flakiness, and it's unclear what their future is. If they are kept at all I expect it will be solely for testing that applies to Bokeh server cases specifically.

For testing BokehJS changes, there are pure JS integration tests here nowadays:

https://github.com/bokeh/bokeh/tree/branch-3.2/bokehjs/test/integration

That's going to be the appropriate place to add tests for this. @mattpap can offer specific guidance if needed.

bryevdv · 2023-04-10T20:50:42Z

bokehjs/src/lib/models/widgets/spinner.ts

@@ -104,9 +105,9 @@ export class SpinnerView extends NumericInputView {
  }

  get precision(): number {
-    const {low, high, step} = this.model
+    const {low, high, step, initial_value_precision} = this.model


Do we actually need to save and and only use the precision from the initial value? Would it not simply work to just always consider the precision of the current value? (if the initial value is a float then adding an int to it should not change its precision, for not-extremely large values at least, and if we are in that regime we won't be able to honor the step anyway)

If I understand you correctly, you mean to apply the precision on this.model.value directly? if so then I actually did do that in the beginning (see first commit a0b749a) but that meant floating errors in some cases, for instance when decrementing, the value would end up showing as 1.299999999998 or so. Please correct me if I am wrong

Ex: Spinner(value=1.3, low=0, high=10, mode="float", step=1)

increment once: value = 2.3
decrement once: value = 1.2999999999999998

Properties are meant for configurability or for computed values of significance to the user, so if that's not the case then this shouldn't be a public property, and possibly a property altogether. If we really need to store this, then a view private member should be sufficient.

If it isn't stored do you suggest doing this another way? As I mentioned , taking the value's precision right away resulted in large precisions, or am I doing something wrong?

Thank you for your feedback

However, I'm not sure if we actually need all of this complexity. What's the point of handling precision in this widget, trying to hide the fact that it uses floating point numbers? If looks are the goal, then one can use a formatter for this. If one actually wants non-fp behavior, then we should provide appropriate number types, which then could be reused in other widgets to get equivalent behavior.

I would definitely be more comfortable with computing an initial (internal) "max precision" if that value fed into a formatter, rather than actually futzing with values.

mattpap · 2023-04-12T10:09:40Z

https://github.com/bokeh/bokeh/tree/branch-3.2/bokehjs/test/integration

That's going to be the appropriate place to add tests for this. @mattpap can offer specific guidance if needed.

Unless we need baselines for this, in particular image baselines, then unit tests are a better place. Both unit and integration tests are a bit of a misnomer in bokehjs due to their history. unit tests are both true unit and integration tests that involve no persistent baselines (*.blf files or image baselines), whereas integration tests are tests that produce and verify baselines. By integration tests in this context, we mean integration within bokehjs. Cross bokeh bokehjs integration tests should be performed in tests/integration, assuming those tests will be restored to function.

msalem99 · 2023-04-12T10:31:30Z

https://github.com/bokeh/bokeh/tree/branch-3.2/bokehjs/test/integration

That's going to be the appropriate place to add tests for this. @mattpap can offer specific guidance if needed.

Unless we need baselines for this, in particular image baselines, then unit tests are a better place. Both unit and integration tests are a bit of a misnomer in bokehjs due to their history. unit tests are both true unit and integration tests that involve no persistent baselines (*.blf files or image baselines), whereas integration tests are tests that produce and verify baselines. By integration tests in this context, we mean integration within bokehjs. Cross bokeh bokehjs integration tests should be performed in tests/integration, assuming those tests will be restored to function.

Hello,
I managed to write tests for this in https://github.com/bokeh/bokeh/blob/branch-3.2/bokehjs/test/integration/widgets.ts
I wasn't sure of commiting though to obtain the CI baselines as I am not sure how you want to move forward with this fix

msalem99 · 2023-04-13T13:59:52Z

I tried to implement a max_precision but after writing unit tests in bokehjs/test/unit/models/widgets/spinner.ts it seems that sometimes the initial value entered has a precision of 16 so fp issues emerge (this can be seen in the failing tests in 0452e44) even from the value inputted by the user. I am not sure why the initial inputted value has a precision of 16 so any feedback is appreciated.
Thank you

bryevdv · 2023-04-13T15:26:43Z

I am not sure why the initial inputted value has a precision of 16 so any feedback is appreciated.

Well, if the initial value is not an exact fp value that seems about right.

I am really tempted to more or less punt on this:

add a public property to control (not compute) the formatting precision
just add the values step values as-is without any other consideration.

As @mattpap suggests, we are more or less trying to magic away the intrinsic behavior of floating point values, and that is just a hopeless task.

cc @bokeh/dev

msalem99 · 2023-04-13T18:59:04Z

I figured maybe I could obtain the max precision through the format_value string by counting the decimals after the dot from the string, this would work but I think it would mean that using the format property for approximating values would be useless since the values are truncated. Not sure if this is considered clean enough. Any feedback would be appreciated.
Thank you

bryevdv · 2023-04-13T22:27:46Z

@msalem99 the more I think and look at this, the more I think perhaps we should simply abandon the precision complexity altogether. Doing a quick survey online:

an old MS API only supported int spinners and advised to use a formatter to divide the presentation by a power of 10 for display
the actual built-in HTML number input with float and step=1 actually behaves they way our existing spinner does AFAICT, i.e it always steps to an integer, regardless of what the start value is!
For arbitrary decimals, must set step=any -- which actually means a step of 1 but allow any decimals in the input value (?!)
Various different weirdness and complains for other JS spinners

I don't think there is any standardization of consistency at all in this space, and every option seems to have some drawback or other.

IIRC all the precision computations came about as a result of some problem with rounding. But we were only rounding because there was no option to add a formatter at that time in the early days. I think now that a format can be supplied, the actual best thing to do is:

rip out all precision and rounding in the spinner. Just add the step as-is to the current value, as-is.

Then the only possible "problem" is that floating point precision may cause the min or max values to be "missed" but that's just how fp works. I think we can just reiterate that fact of life in the docs, and not worry about it.

msalem99 · 2023-04-13T23:40:02Z

rip out all precision and rounding in the spinner. Just add the step as-is to the current value, as-is.

Thank you for your feedback, I think this is definitely the most suitable course of action, I was hoping I could implement the max_precision correctly to use it as a default format if a format is not given but I am not sure how to do that since the precision function that returns max is essentially useless since the step could also be a floating point, same goes for the high and low.

any suggestion regarding a default format? or just leave it as fp incase no format is provided?

Thank you

jbednar · 2023-04-15T04:11:26Z

I agree with making it simpler, but I worry if floating point precision issues could cause the value to exceed the upper bound by even the smallest amount. E. g. if the value is used to compute an index into an array, now there might be out of bounds access, despite the spinner bounds seemingly being safe against that. Send like that could lead to some dangerous apps!

bryevdv · 2023-04-15T16:18:59Z

@jbednar by "miss" the max/min values, I meant "might never be able to attain those values" (even if in a "perfect math sense" they should be able to). I agree that a strict comparison against min/max is always needed to prevent any overshoot or undershoot due to fp precision loss.

bryevdv · 2023-04-15T16:22:43Z

@msalem99 I think improvements to default formatting is a perfectly reasonable thing to look at but let's save it for a follow-on issue separate from this one

msalem99 · 2023-04-15T17:22:10Z

I am not sure where to go from here regarding this fix. Also I am also not sure why the Bokeh-CI macos unit-tests and windows minimal-deps are failing, any feedback is appreciated.

Thank you.

bryevdv · 2023-04-15T21:47:00Z

@msalem99 I don't think the failures are related to your PR. The documentation job is failing everywhere due to a recent downstream package update. I think the others were conda env network failures. I've restarted all the failed jobs.

I am going to look over the PR (especially tests) tomorrow but I think it should be close and/or ready

bryevdv · 2023-04-24T01:06:20Z

bokehjs/test/unit/models/widgets/spinner.ts

+    expect(input.value).to.be.equal("4.36")
+
+    await mousedown_mouseup(increment_button)
+    expect(input.value).to.be.equal("5.00")


Is this the desired behaviour? I think I might have expected that in this case the increment button simply had no effect at all. Perhaps it should be configurable whether a "partial step" is allowed at the bounds (to exactly attain the bounds) or whether the step is strictly respected at all times (even if it means the bounds values can never be attained)

Same questions for lower bound, of course.

cc @bokeh/dev

in-case of the presence of a reach_bounds property and set to true, if a partial step is made to reach the bound exactly, should this partial step be preserved in the followed opposite step and then back to the original step? or should the original step be applied right away?
ex:
high: 5 step:0.5 value 4.7
increment -> 5
2x decrement -> 4.7 , 4.2 or 4.5 , 4.0?

@bokeh/dev thoughts on a preference? It could be made configurable I suppose, but I'd rather just pick a good default to start and only add a configuration if it becomes needed.

bryevdv

Hi @msalem99 apologies for the delay, I have not had as much time for Bokeh in the last few months. I'd like to get this merged for 3.3 so that we can also add the formatter option along with it. Can you fix the small merge conflict, and submit an issue regarding adding a formatting property to the spinner?

bokehjs/src/lib/models/widgets/spinner.ts

Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>

msalem99 · 2023-11-04T22:51:37Z

Hello @bryevdv,
Is there any chance this could still be considered?
Thanks.

bryevdv · 2023-11-05T02:05:45Z

Thanks for you patience @msalem99 the tests all look good to me so i will go ahead and merge this now.

* Added value precision check when obtaining the precision * Improved the initial fix and Added tests * Added initial_value_precision to the Python Spinner module * Added internal max precision member in the spinner module * Added unit tests for spinner widget * obtain initial precision of initial value from the inputted num string * Removed all precision computation from Spinner model in bokehjs * override reach_bounds in spinner * Update bokehjs/src/lib/models/widgets/spinner.ts Co-authored-by: Bryan Van de Ven <bryan@bokeh.org> * fix linting errors --------- Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>

github-actions · 2024-10-25T19:10:32Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

msalem99 added 2 commits April 10, 2023 11:46

Added value precision check when obtaining the precision

a0b749a

Improved the initial fix and Added tests

4cb0116

bryevdv reviewed Apr 10, 2023

View reviewed changes

Added initial_value_precision to the Python Spinner module

f24fbaa

mattpap added the status: WIP label Apr 12, 2023

msalem99 added 2 commits April 13, 2023 13:04

Added internal max precision member in the spinner module

de26927

Added unit tests for spinner widget

0452e44

obtain initial precision of initial value from the inputted num string

9b12e93

Removed all precision computation from Spinner model in bokehjs

a44e004

bryevdv added this to the 3.2 milestone Apr 24, 2023

bryevdv reviewed Apr 24, 2023

View reviewed changes

msalem99 added 2 commits May 13, 2023 01:07

Merge branch 'bokeh:branch-3.2' into 13064_fix_spinner_precision

3961390

override reach_bounds in spinner

1c5ad20

msalem99 requested a review from bryevdv May 13, 2023 00:03

mattpap modified the milestones: 3.2, 3.3 Jun 9, 2023

mattpap changed the base branch from branch-3.2 to branch-3.3 June 15, 2023 07:45

bryevdv reviewed Jul 13, 2023

View reviewed changes

bokehjs/src/lib/models/widgets/spinner.ts Outdated Show resolved Hide resolved

msalem99 and others added 3 commits July 15, 2023 05:53

Merge branch 'branch-3.3' into 13064_fix_spinner_precision

4fdf35a

Update bokehjs/src/lib/models/widgets/spinner.ts

e8f0f19

Co-authored-by: Bryan Van de Ven <bryan@bokeh.org>

fix linting errors

b2d978f

bryevdv modified the milestones: 3.3, 3.x Oct 5, 2023

mattpap changed the base branch from branch-3.3 to branch-3.4 October 10, 2023 12:47

Merge branch 'branch-3.4' into 13064_fix_spinner_precision

bf45eb3

msalem99 requested a review from bryevdv November 4, 2023 22:42

bryevdv approved these changes Nov 5, 2023

View reviewed changes

bryevdv added status: accepted and removed status: WIP labels Nov 5, 2023

bryevdv modified the milestones: 3.x, 3.4 Nov 5, 2023

bryevdv merged commit 0ba4712 into bokeh:branch-3.4 Nov 5, 2023

bryevdv mentioned this pull request Nov 5, 2023

More 3.3.1 backports #13512

Merged

bryevdv modified the milestones: 3.4, 3.3.1 Nov 5, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix spinner precision #13078

fix spinner precision #13078

msalem99 commented Apr 10, 2023 •

edited

Loading

codecov bot commented Apr 10, 2023 •

edited

Loading

bryevdv commented Apr 10, 2023 •

edited

Loading

bryevdv Apr 10, 2023 •

edited

Loading

msalem99 Apr 10, 2023 •

edited

Loading

msalem99 Apr 10, 2023

mattpap Apr 12, 2023

msalem99 Apr 12, 2023

mattpap Apr 12, 2023

bryevdv Apr 12, 2023 •

edited

Loading

mattpap commented Apr 12, 2023

msalem99 commented Apr 12, 2023

msalem99 commented Apr 13, 2023

bryevdv commented Apr 13, 2023

msalem99 commented Apr 13, 2023

bryevdv commented Apr 13, 2023 •

edited

Loading

msalem99 commented Apr 13, 2023 •

edited

Loading

jbednar commented Apr 15, 2023

bryevdv commented Apr 15, 2023 •

edited

Loading

bryevdv commented Apr 15, 2023

msalem99 commented Apr 15, 2023

bryevdv commented Apr 15, 2023

bryevdv Apr 24, 2023

msalem99 Apr 25, 2023 •

edited

Loading

bryevdv May 12, 2023

bryevdv left a comment

msalem99 commented Nov 4, 2023

bryevdv commented Nov 5, 2023

github-actions bot commented Oct 25, 2024

fix spinner precision #13078

fix spinner precision #13078

Conversation

msalem99 commented Apr 10, 2023 • edited Loading

codecov bot commented Apr 10, 2023 • edited Loading

Codecov Report

bryevdv commented Apr 10, 2023 • edited Loading

bryevdv Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

msalem99 Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

msalem99 Apr 10, 2023

Choose a reason for hiding this comment

mattpap Apr 12, 2023

Choose a reason for hiding this comment

msalem99 Apr 12, 2023

Choose a reason for hiding this comment

mattpap Apr 12, 2023

Choose a reason for hiding this comment

bryevdv Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

mattpap commented Apr 12, 2023

msalem99 commented Apr 12, 2023

msalem99 commented Apr 13, 2023

bryevdv commented Apr 13, 2023

msalem99 commented Apr 13, 2023

bryevdv commented Apr 13, 2023 • edited Loading

msalem99 commented Apr 13, 2023 • edited Loading

jbednar commented Apr 15, 2023

bryevdv commented Apr 15, 2023 • edited Loading

bryevdv commented Apr 15, 2023

msalem99 commented Apr 15, 2023

bryevdv commented Apr 15, 2023

bryevdv Apr 24, 2023

Choose a reason for hiding this comment

msalem99 Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

bryevdv May 12, 2023

Choose a reason for hiding this comment

bryevdv left a comment

Choose a reason for hiding this comment

msalem99 commented Nov 4, 2023

bryevdv commented Nov 5, 2023

github-actions bot commented Oct 25, 2024

msalem99 commented Apr 10, 2023 •

edited

Loading

codecov bot commented Apr 10, 2023 •

edited

Loading

bryevdv commented Apr 10, 2023 •

edited

Loading

bryevdv Apr 10, 2023 •

edited

Loading

msalem99 Apr 10, 2023 •

edited

Loading

bryevdv Apr 12, 2023 •

edited

Loading

bryevdv commented Apr 13, 2023 •

edited

Loading

msalem99 commented Apr 13, 2023 •

edited

Loading

bryevdv commented Apr 15, 2023 •

edited

Loading

msalem99 Apr 25, 2023 •

edited

Loading