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

Streamline constants #56

Closed
wants to merge 2 commits into from
Closed

Streamline constants #56

wants to merge 2 commits into from

Conversation

valarauca
Copy link
Contributor

Changes:

  • Geodesic fields were are constants (not dependent on spheroid parameters) are now just constants.
  • Fixed the type of many constants (i64 -> usize) where applicable.
  • Remove arguments that are constants.
  • Remove casts that were made redunant by changing i64 -> usize.
  • Rewrite polyval; The new version is as close to optimal as my local

This is the first patch in a series of PR aiming at improving performance of geodesic inverse calculation.

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Changes:

* Geodesic fields were are constants (not dependent on spheroid
  parameters) are now just constants.
* Fixed the type of many constants (i64 -> usize) where applicable.
* Remove arguments that are constants.
* Remove casts that were made redunant by changing i64 -> usize.
* Rewrite polyval; The new version is as close to optimal as my local

This is the first patch in a series of PR aiming at improving
performance of geodesic inverse calculation.
@valarauca
Copy link
Contributor Author

Minor changes to a few functions in geomath, sorry. Got a little distracted while copying & pasting stuff.

Copy link
Contributor Author

@valarauca valarauca left a comment

Choose a reason for hiding this comment

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

A few nitpicks I spotted last night while looking this over.

src/geodesic.rs Outdated Show resolved Hide resolved
src/geodesic.rs Outdated Show resolved Hide resolved
src/geodesic.rs Outdated Show resolved Hide resolved
@michaelkirk
Copy link
Member

There are some nice perf wins here, thanks!

$ cargo bench --bench="*" --  --baseline=main-2024-01-24 
   Compiling geographiclib-rs v0.2.3 (/Users/mkirk/src/georust/geographiclib-rs)
    Finished bench [optimized] target(s) in 1.03s
     Running benches/geodesic_benchmark.rs (target/release/deps/geodesic_benchmark-656e1aa161ec181c)
direct (c wrapper)/default
                        time:   [23.868 µs 23.894 µs 23.923 µs]
                        change: [-0.1047% +0.2586% +0.8488%] (p = 0.32 > 0.05)
                        No change in performance detected.
Found 6 outliers among 100 measurements (6.00%)
  3 (3.00%) high mild
  3 (3.00%) high severe

direct (rust impl)/default
                        time:   [26.527 µs 26.565 µs 26.609 µs]
                        change: [-11.679% -11.528% -11.381%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 8 outliers among 100 measurements (8.00%)
  1 (1.00%) low mild
  3 (3.00%) high mild
  4 (4.00%) high severe

inverse (c wrapper)/default
                        time:   [44.686 µs 44.742 µs 44.804 µs]
                        change: [-2.2765% -0.7738% +0.2501%] (p = 0.30 > 0.05)
                        No change in performance detected.
Found 6 outliers among 100 measurements (6.00%)
  2 (2.00%) high mild
  4 (4.00%) high severe

inverse (rust impl)/default
                        time:   [58.362 µs 58.459 µs 58.567 µs]
                        change: [-21.016% -20.859% -20.699%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) high mild
  1 (1.00%) high severe

This PR entails a lot of different changes. Would you be willing to break it up a bit to be easier to review?

@valarauca
Copy link
Contributor Author

valarauca commented Jan 24, 2024

Would you be willing to break it up a bit to be easier to review?

I already did or I hope I did? 😅 my eventual goal was to wind up something like this.

I guess I can try to subdivide this further. I am doing 2 different things in 1 PR (standardizing constants & changing i64 to usize).

Let me see if I can reduce the scope slightly

@valarauca valarauca closed this Jan 24, 2024
@valarauca valarauca mentioned this pull request Jan 24, 2024
2 tasks
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