Skip to content

Simplify units before returning numeric value#902

Merged
josdejong merged 3 commits into
josdejong:developfrom
AlexanderBeyn:simplify-units-in-toNumeric
Aug 12, 2017
Merged

Simplify units before returning numeric value#902
josdejong merged 3 commits into
josdejong:developfrom
AlexanderBeyn:simplify-units-in-toNumeric

Conversation

@AlexanderBeyn
Copy link
Copy Markdown
Contributor

This should close #901 . There is a separate issue breaking unit tests when run all at once, which I'll be filing shortly.

@AlexanderBeyn
Copy link
Copy Markdown
Contributor Author

#903 explains the test issue, which appears as

  Unit
    constructor
      ✓ should create square meter correctly
    toNumeric
      1) should simplify units before returning a numeric value


  1 passing (33ms)
  1 failing

  1) Unit toNumeric should simplify units before returning a numeric value:

      AssertionError [ERR_ASSERTION]: 1e-8 deepEqual 0.01
      + expected - actual

      -1e-8
      +0.01
      
      at Context.<anonymous> (test/type/unit/Unit.test.js:259:14)

@josdejong
Copy link
Copy Markdown
Owner

Thanks @AlexanderBeyn for this fix ! And sorry for the late reply - I'm just back from holidays.

One question: shouldn't this.simplifyUnitListLazy() be called on top of the toNumeric() method instead of halfway? Or call other.simplifyUnitListLazy() instead of this.simplifyUnitListLazy()? Since we're dealing with other only in this method, and other can be replace in the first
if (valuelessUnit) {...} statement.

@AlexanderBeyn
Copy link
Copy Markdown
Contributor Author

You're absolutely right. I've updated the pull request to use other instead of this.

@josdejong
Copy link
Copy Markdown
Owner

Thanks for updating.

The code looks ok, though the unit test that you've added for this specific use case is failing, can you check that out?

@AlexanderBeyn
Copy link
Copy Markdown
Contributor Author

I've changed to use mass squared instead of length squared for the units, but this test still feels brittle because of the issue described in #903.

@josdejong
Copy link
Copy Markdown
Owner

josdejong commented Aug 12, 2017

Thanks for the fix. The non-deterministic behavior of units indeed make unit tests a bit fragile :(

@josdejong josdejong merged commit a65420c into josdejong:develop Aug 12, 2017
@AlexanderBeyn AlexanderBeyn deleted the simplify-units-in-toNumeric branch August 14, 2017 20:09
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.

Unit's .toNumeric() incorrect immediately after calculation with mixed units

2 participants