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

Fix rounding algorithm #958

Merged
merged 6 commits into from
Jun 13, 2022
Merged

Conversation

OrigamiDream
Copy link
Contributor

♻️ Current situation

Related:

💡 Proposed solution

As @adriancable have mentioned in #956, the previous solution may have possible incorrect behaviors.
So I have tried all possible tests which can be happened in 0 and 100.

I moved the rounding logic in front of clamping logic.
Plus, to fix in case the error is smaller than epsilon and greater than 0.5, I have changed Math.round() to Math.floor().
We don't need round the value in server-side, due to Apple Home app will always automatically round the value fit to step size.

⚙️ Release Notes

➕ Additional Information

Testing

I have rewritten entire test code related with this issue.
The minStep in the final test only uses (100/6), of course I have tried other combinations like 3, 7, 9.
All cases have passed, including approaches which @adriancable have mentioned before.

The test takes up to 3 minutes per step, this is the reason why I have decided to use only 100/6.

Reviewer Nudging

I would appreciate if you have better idea.

@coveralls
Copy link

coveralls commented Jun 5, 2022

Pull Request Test Coverage Report for Build 2450189177

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 51.683%

Totals Coverage Status
Change from base Build 2405678714: -0.09%
Covered Lines: 5715
Relevant Lines: 10090

💛 - Coveralls

@Supereg
Copy link
Member

Supereg commented Jun 5, 2022

@adriancable would you mind reviewing this PR?

@OrigamiDream OrigamiDream changed the title Fix floating point precision Fix rounding algorithm Jun 6, 2022
@OrigamiDream
Copy link
Contributor Author

The failure is originated from the new rounding algorithm. Im gonna fix this after WWDC 🥳

@adriancable
Copy link
Contributor

It’s not sufficient to check that stepValue != null. It could also be undefined or 0. Either will lead to an overflow which is what happens in your test.

@OrigamiDream
Copy link
Contributor Author

OrigamiDream commented Jun 6, 2022

We should not handle stepValue which is undefined or 0.
The original input value must not be changed in this case.

And the test failure is not a kind of overflow.
They are just the wrong 'numericMin' handling errors in the old test cases.

@adriancable
Copy link
Contributor

I think this looks OK now.

Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. There were some other edge cases discussed in the issue form right? Would it make sense to add them to the suite of test cases to avoid any future regressions?

src/lib/Characteristic.spec.ts Show resolved Hide resolved
@OrigamiDream
Copy link
Contributor Author

Possibly one edge case.

Basically all characteristics have minValue itself, so there would no failure if minValue is valid.
However somehow the plugin developer set the minValue as undefined ignoring default values, the minValue will be changed to -Number.MAX_VALUE.
Eventually, the rounding algorithm would work wrongly. (It would feel like unexpected behavior to the plugin developer)

Although this can be happened, it is more like the developer sets non-null properties as null which cannot be detected in compile-time.
Therefore, I have decided to not add warning signs about this. (And too many affected test cases which are spying on characteristicWarning)

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.

4 participants