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

Improve rotation stability #89

Merged
merged 5 commits into from
Dec 17, 2018
Merged

Conversation

nbraud
Copy link
Collaborator

@nbraud nbraud commented Dec 11, 2018

  • More accurate implementation of cos and sin, solves precision issue from Improve the Readme #87.
  • Tighten tolerances on trigonometry stability tests.
  • Expect exact results in unit tests, when applicable.

@nbraud nbraud mentioned this pull request Dec 11, 2018
4 tasks
@nbraud
Copy link
Collaborator Author

nbraud commented Dec 11, 2018

PS: There's probably a performance hit in Vector2.rotate. Do we care?

@AstraLuma
Copy link
Member

Can you categorize the performance hit?

Copy link
Member

@AstraLuma AstraLuma left a comment

Choose a reason for hiding this comment

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

Most of these are non-functional stylistic changes.

ppb_vector/vector2.py Outdated Show resolved Hide resolved
ppb_vector/vector2.py Outdated Show resolved Hide resolved
ppb_vector/vector2.py Outdated Show resolved Hide resolved
ppb_vector/vector2.py Outdated Show resolved Hide resolved
ppb_vector/vector2.py Show resolved Hide resolved
ppb_vector/vector2.py Outdated Show resolved Hide resolved
@nbraud
Copy link
Collaborator Author

nbraud commented Dec 11, 2018

@astronouth7303 I think the commit whose style you find problematic is 5def680 ; it moved imports to ensure trig functions aren't used directly outside of Vector2.{trig,angle}.

@nbraud
Copy link
Collaborator Author

nbraud commented Dec 11, 2018

PS: Tell me when the review is OK, and I will clean up the history.

@AstraLuma
Copy link
Member

That is the big one, although .trig() should be ._trig() to mark internal API.

@nbraud
Copy link
Collaborator Author

nbraud commented Dec 11, 2018

Yeah, I did that change too.

@nbraud nbraud force-pushed the rotation-stability branch from 04c2dd5 to 219e874 Compare December 11, 2018 21:17
@nbraud
Copy link
Collaborator Author

nbraud commented Dec 11, 2018

OK, this is rebased & cleaned up

@nbraud
Copy link
Collaborator Author

nbraud commented Dec 12, 2018

Re: performance hit, I ran a pretty basic test (python3 -m perf timeit -s 'from ppb_vector import Vector2; v = Vector2(1, 0)' 'v.rotate(30)'):
On master: 1.58 us +- 0.08 us
On this branch: 2.42 us +- 0.14 us

I don't think we should care about a <1μs slowdown, even though that's 52% slower, considering that it gives less surprising results.

PS: Updated to use the perf package, which provides more accurate measurements and std deviation.
I also tried to test this under pypy, but it turns out it can't cope with type annotations, at least with the version packaged in Debian.

@nbraud
Copy link
Collaborator Author

nbraud commented Dec 15, 2018

What's the blocker on this?

@nbraud
Copy link
Collaborator Author

nbraud commented Dec 15, 2018

Unrelated CI failure, due to a missing rel_to in test_scalar_linear

This solves an issue exposed in ppb#87, where imprecisions results in non-null
components where the user would expect 0, for instance:

    >>> Vector2(1, 0).rotate(90)
    Vector2(6.123233995736766e-17, 1.0)
Otherwise, Hypothesis tests it with very large loop values,
slowing down the entire testsuite, and triggering a warning.
@nbraud nbraud force-pushed the rotation-stability branch from f2e7762 to 152c9ab Compare December 16, 2018 20:04
@AstraLuma AstraLuma merged commit aba4d12 into ppb:master Dec 17, 2018
@nbraud nbraud deleted the rotation-stability branch December 17, 2018 17:26
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.

2 participants