8000 Fix Haversine formula in position class (Datatypes.py) by diego-garro · Pull Request #110 · python-metar/python-metar · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@diego-garro
Copy link
Contributor
@diego-garro diego-garro commented Sep 29, 2020

The correct formula has sin²(angle).

Copy link
Collaborator
@phobson phobson left a comment

Choose a reason for hiding this comment

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

@diegogarromolina thanks for the PR. The CI failure you're seeing is related to our python 3.7 Travis configuration and not your PR.

Could you add a test to the test suite that confirms that this is producing the desired results?

@diego-garro
Copy link
Contributor Author
diego-garro commented Sep 30, 2020

Sorry, but the documentation is not clear at position class. Long and Lat in the constructor are floats in degrees I guess, but to compute a in getdistance() method, the functions cos and sin needs a value in radians. I seen that you don't make a test for position before in test folder. Do you tested that computes? I take this repo as an inspiration to develop a metar parser in Dart language, and I seen the error on Haversine formula while writing the Position class in that language. Thanks.

@tomp
Copy link
Member
tomp commented Sep 30, 2020

The methods of the datatypes.position class are dead code, really. They have never been used for anything. I would be inclined to delete the getdistance() and getdirection() methods, rather than fix them. Keeping them around just creates a maintenance burden - it doesn't benefit users of this module.

@diego-garro
Copy link
Contributor Author
diego-garro commented Sep 30, 2020

Like I said above, I develop the correct computes in Dart, but you got a good point, they have never been used for anything. If you delete them, then I don't make a PR. Thanks.

You can follow the repo in Dart here:
https://github.com/diegogarromolina/metart

You inspired this project :)

The correct formula has sin²(anlge).
@akrherz
Copy link
Collaborator
akrherz commented Dec 7, 2020

I rebased this just to see what CI / codecov says. I am inclined to take this PR and then cull the unused API with a subsequent PR. We likely don't have any contract with what our library's public API is and we are still at version 0.x :)

@codecov-io
Copy link

Codecov Report

Merging #110 (73569e6) into master (e572a37) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #110   +/-   ##
=======================================
  Coverage   87.41%   87.41%           
=======================================
  Files           4        4           
  Lines        1049     1049           
=======================================
  Hits          917      917           
  Misses        132      132           
Impacted Files Coverage Δ
metar/Datatypes.py 84.37% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e572a37...73569e6. Read the comment docs.

@akrherz akrherz merged commit df5d289 into python-metar:master Dec 7, 2020
@akrherz akrherz added this to the 1.8 milestone Dec 7, 2020
@akrherz akrherz mentioned this pull request Jan 11, 2021
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.

5 participants

0