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

Resolve clippy warnings and errors #45

Merged
merged 24 commits into from
Sep 1, 2022
Merged

Resolve clippy warnings and errors #45

merged 24 commits into from
Sep 1, 2022

Conversation

Quba1
Copy link
Contributor

@Quba1 Quba1 commented Aug 13, 2022

  • 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.

This PR solves all Clippy warnings and errors present in the codebase. It also silences Clippy warnings that I evaluated as false-positives (eg. excessive_precision) or that required too significant changes (eg. too_many_arguments).

Changes do not seem to have any significant impact on the performance, but they solve some issues mentioned in #32 so this PR may be a start towards performance improvement.

The PR is split into so many commits for easier review, but it should probably be stashed after the review.

@@ -842,7 +849,7 @@ impl Geodesic {
}

let alp12: f64;
if !meridian && comg12 > -0.7071 && sbet2 - sbet1 < 1.75 {
if !meridian && comg12 > -FRAC_1_SQRT_2 && sbet2 - sbet1 < 1.75 {
Copy link
Member

Choose a reason for hiding this comment

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

I understand why you're doing this, but it diverges from our reference implementation whose test suite we validate against.

The tests seem to all still be passing, which makes me wonder if this edge case is maybe not tested too precisely. It could be a real pain to track down later if the tests input changes to test around this more precisely.

Still, your proposed change feels like the right thing to do. Could you add a comment to the effect of:

we're diverging from Karney's implementation here which uses the hardcoded constant: -0.7071 for FRAC_1_SQRT_2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same doubts seeing this suggestion from Clippy. But I decided that it is beneficial to do that because use of internal constant improves code readability and has an explicit type.

This edge case might indeed not be tested precisely enough. Actually, probability for hitting a different branch in Rust implementation than in original implementation is very low: difference between constants is roughly -6.78e-6 and there are two other conditions alongside.

And the constant -0.7071 is present only in Karney's implementation but not in the reference paper. So it seems like its use is an implementation shortcut.

I've added the requested change.

Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

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

One comment nit, but otherwise, LGTM, thanks!

@Quba1
Copy link
Contributor Author

Quba1 commented Aug 20, 2022

I've also added a few common trait implementations using derive macro for Geodesic and GeodesicLine. It's mostly a QoL change but it also satisfies one of the Rust API Guidelines Checklist rules (C-COMMON-TRAITS). And this PR feels like a good place to add this change.

@michaelkirk
Copy link
Member

I've also added a few common trait implementations using derive macro for Geodesic and GeodesicLine. It's mostly a QoL change but it also satisfies one of the Rust API Guidelines Checklist rules (C-COMMON-TRAITS). And this PR feels like a good place to add this change.

The derived Default implementation seems strange to me - please remove it.

There are so many parameters in that struct, many which have a particular relationship. It seems extremely unlikely that anything reasonable could be made by having them all equal 0, as the derived implementation does.

@Quba1
Copy link
Contributor Author

Quba1 commented Sep 1, 2022

I'm sorry, my oversight. I've removed the Default traits.

@michaelkirk michaelkirk merged commit 1358b13 into georust:master Sep 1, 2022
@michaelkirk
Copy link
Member

Thanks!

@Quba1 Quba1 mentioned this pull request Dec 29, 2022
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