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

splitUnit() has some strange behavior. #865

Open
jcheroske opened this Issue Jun 2, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@jcheroske

jcheroske commented Jun 2, 2017

I'm trying to convert from inches to feet/inches using splitUnit() and format. Here is an example converter:

const inchesToFeetAndInches = valueInInches => 
  math.unit(valueInInches, 'in')
    .splitUnit(['ft', 'in'])
    .map(u => u.format({notation: 'fixed', precision: 0}))
    .join(' ')

Here is some input/output I'm seeing:
12 -> 0 ft 12 in // would rather see 1 ft 0 in
24 -> 1 ft 12 in // same issue
36 -> 3 ft -0 in // negative zero inches???
60 -> 5 ft 0 in // it worked!

There's some non-deterministic behavior happening.

@ericman314

This comment has been minimized.

Collaborator

ericman314 commented Jun 2, 2017

This is caused by round-off error. Units are represented internally in SI units, and we're limited by floating point precision, so the conversion from inches to meters and back to inches can cause errors. In the first case, 12 inches becomes 11.999999999999998 inches. This gets split into 0 ft 11.999999999999998 in. The formatter displays this as 0 ft 12 in.

A possible solution is to introduce a fudge factor so that when the units are split very close to an integer boundary, it rounds up instead of down. So you would get 1 ft 0 in instead of 0 ft 12 in. @josdejong, what do you think?

@jcheroske

This comment has been minimized.

jcheroske commented Jun 3, 2017

Well, I'd love to see the formatter deal with this issue internally and just produce pretty output, so whatever needs to happen to make that happen is ok with me! Thank you for the explanation though. It's good for me to understand that the lib is converting back and forth between SI units.

@josdejong

This comment has been minimized.

Owner

josdejong commented Jun 4, 2017

A possible solution is to introduce a fudge factor so that when the units are split very close to an integer boundary, it rounds up instead of down.

Yes I think we should do something like that. Round for example to 12 or 14 digits internally to prevent these round-off errors from popping up.

@ericman314

This comment has been minimized.

Collaborator

ericman314 commented Jun 4, 2017

Basically something that might be called fixFuzzy which rounds toward zero unless we are already close enough to an integer, in which case we round to that integer. But how close is close enough will change based on the order of magnitude of the floating-point number and number of decimal digits of a BigNumber.

I don't necessarily want to pollute the library with extraneous functions, but could fixFuzzy be useful generally? Or ceilFuzzy and floorFuzzy?

@josdejong

This comment has been minimized.

Owner

josdejong commented Jun 4, 2017

Just thinking aloud: it may be possible to do something similar to what we have for comparing equality: we compare whether two values are almost equal given a configurable epsilon. Could we apply something similar to fix? See whether when rounding up the difference is smaller than epsilon?

@ericman314

This comment has been minimized.

Collaborator

ericman314 commented Jun 4, 2017

@ericman314

This comment has been minimized.

Collaborator

ericman314 commented Jun 5, 2017

The implementation of nearlyEqual is almost--but not quite--sufficient to avoid the weird behavior described by @jcheroske. Here's why:

splitUnit(36 inch, [ft, in])
  [3 ft, -4.3709567898628e-15 in]

splitUnit(36000 inch, [ft, in])
  [3000 ft, -4.4758597528195e-12 in]

Notice that the error in the last value scales with the input value, so checking against a constant EPSILON only works if the input value is small. nearlyEqual works fine when evaluating x == y for two large numbers that are nearly equal, but it doesn't (and we shouldn't expect it to) work when comparing x - x == y - x, since in the latter case we are comparing two small numbers that have what is now a very large relative error.

So it will be a bit more work than I originally thought.

@ericman314

This comment has been minimized.

Collaborator

ericman314 commented Jun 5, 2017

OK, I've got a solution. nearlyEquals works fine until checking the remainder (the little bit of inches left over). Instead of comparing the remainder with 0, splitUnit now compares the original value with the sum of all the parts excluding the remainder. If those are nearly equal, it assumes the remainder can be safely set to exactly 0.

See #867

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment