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

'floor_in' can provide confusing results #589

Open
ValentinLeTallec opened this issue Sep 29, 2024 · 7 comments
Open

'floor_in' can provide confusing results #589

ValentinLeTallec opened this issue Sep 29, 2024 · 7 comments
Labels

Comments

@ValentinLeTallec
Copy link
Contributor

Summary

floor_in can provide confusing results (most likely due to a floating point precision problem)

Steps to reproduce

Link to the steps in numbat's web interface

Expected Results

>>> let t = 1.5 weeks
>>> let t2 = t - (t |> floor_in(days)) -> hours
>>> t2
      = 12 h    [Time]
>>> t2 |> floor_in(hours)
      = 12 h

Actual Results

>>> let t = 1.5 weeks
>>> let t2 = t - (t |> floor_in(days)) -> hours
>>> t2
      = 12 h    [Time]
>>> t2 |> floor_in(hours)
      = 11 h

Setup

  • Numbat v1.13.0
@sharkdp
Copy link
Owner

sharkdp commented Sep 29, 2024

Thank you for reporting this.

most likely due to a floating point precision problem

Yes. The problem is here: t - (t |> floor_in(days)). t is in weeks and t |> floor_in(days) is in days. So the subtraction is effectively 1.5 weeks - 10 days. In a subtraction like this, Numbat converts the right-hand-side quantity to the unit of the left-hand-side quantity. So you end up with 0.0714286 weeks here, which you then convert back to hours. This leads to a result that is slightly less (< 1 nanosecond) than 12 hours.

Instead, what you should to is to perform the computation in days:

>>> let t = 1.5 weeks
>>> let t2 = (t -> days) - (t |> floor_in(days))
>>> t2 |> floor_in(hours)

    = 12 h    [Time]

I don't really think this is a bug in Numbat. It's rather a floating point precision problem, as you pointed out. If you think that there is something we can improve, please let me know.

@ValentinLeTallec
Copy link
Contributor Author

ValentinLeTallec commented Sep 29, 2024

I have a few ideas, but, they are not so great :

  • Having some rounding depending on the unit could help, but that seems like a slippery slope. Moreover, it only makes some kind of sense for a few units like weeks or hour but no so much for seconds or meters.
  • Changing the implementation of floor_in to add a few 10^-9 of the unit could work, but it does not really deliver on the implicit promise of just flooring the number given (though for flooring, a few 10^-9 should not have impact in the vast majority of cases).

@Goju-Ryu
Copy link
Contributor

Another approach could be to change the subtraction. Instead of using the left hand side to get the unit, we could always use the smaller unit instead.
I haven't fully thought out the potential consequences of this, so maybe it isn't worth the downsides, but that would seem natural to me compared to order of units being significant.

@rben01
Copy link
Contributor

rben01 commented Sep 30, 2024

Another approach could be to change the subtraction

My first thought as well. 1/7 weeks is prone to floating point issues that 7 days is not. Going through the smaller of the two units seems reasonable, at least in the case when the ratio between them is an integer. (If it's something like 5/3, then you'll have floating point issues in either direction, although I don't see one direction being any worse than the other so this logic still might as well be applied universally.)

@ValentinLeTallec
Copy link
Contributor Author

After doing the operation with the smallest of the two unit, would it then be better to return the result as this unit or to keep the current exposed behavior and return the result in the unit of the left-hand-side quantity ?

@rben01
Copy link
Contributor

rben01 commented Sep 30, 2024

There is perhaps a third option which is to move from floats to rationals where possible. Then we'd have 1.5 weeks = 1.5*7 days = 21/2 days, and from there all the math works out fine because it's exact. Not sure how big a lift this would be. Even just recognizing when the user’s input is “rational enough” sounds challenging — 1.5? Yes. 0.1248? Probably not.

@sharkdp
Copy link
Owner

sharkdp commented Oct 9, 2024

Another approach could be to change the subtraction. Instead of using the left hand side to get the unit, we could always use the smaller unit instead.
I haven't fully thought out the potential consequences of this, so maybe it isn't worth the downsides, but that would seem natural to me compared to order of units being significant.

Yes, that is a much better approach 👍. This also restores the (anti)commutative property of addition and subtraction, which is violated by the current behavior.

After doing the operation with the smallest of the two unit, would it then be better to return the result as this unit or to keep the current exposed behavior and return the result in the unit of the left-hand-side quantity ?

To restore (anti)commutativity, it should always return results in the same unit, no matter if we write a + b or b + a.

There is perhaps a third option which is to move from floats to rationals where possible.

Relevant issue: #118. See also this comment: #180 (comment)

@sharkdp sharkdp changed the title [bug] floor_in can provide confusing results 'floor_in' can provide confusing results Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants