Skip to content

Add handling with extra precision in lat/long #80

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jun 17, 2022
Merged

Add handling with extra precision in lat/long #80

merged 5 commits into from
Jun 17, 2022

Conversation

gamblor21
Copy link
Member

Addresses issue #63 by adding two new latitude and two new longitude attributes for degrees and minutes. When combined as one value the current CP float limit cannot handle the full precision of latitude or longitude.

The _parse_degrees function was modified to give more precision to the existing latitude and longitude values (some was lost in the calculation not in the final storage) but that value is still imprecise.

@tekktrik tekktrik requested a review from a team June 16, 2022 14:28
dhalbert
dhalbert requested changes Jun 16, 2022
View reviewed changes
Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Hi, thanks for doing this. I have some touch-ups.

@caternuson what do you think of the API?

@dhalbert dhalbert requested a review from caternuson June 16, 2022 14:47
@caternuson
Copy link
Contributor

@dhalbert lgtm. seems like idea is _degrees and _minutes provide integer values to avoid potential precision issue when converting to single latitude float value? could maybe have something like a named tuple instead of two separate variables? but probably not worth the extra effort.

@gamblor21
Copy link
Member Author

@dhalbert lgtm. seems like idea is _degrees and _minutes provide integer values to avoid potential precision issue when converting to single latitude float value? could maybe have something like a named tuple instead of two separate variables? but probably not worth the extra effort.

That is the idea behind it. Float just does not have the precision required to hold all the information. I saw something else use a tuple later but that is my lack of python experience showing through.

dhalbert
dhalbert approved these changes Jun 17, 2022
View reviewed changes
Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I think having .latitude_degrees and .latitude_minutes separately is OK. If we made a tuple it would probably be broken apart by the calling code anyway.

One thing adding the attributes points out is that the documentation is lacking: there is no documentation for .latitude, .timestamp_utc, etc., already, and now it needs it even more. But that can be a separate issue.

@dhalbert dhalbert merged commit 583fbd0 into adafruit:main Jun 17, 2022
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jun 18, 2022
@adafruit-adabot
Automated update by Adabot (adafruit/adabot@77089da)
2728261 
Updating https://github.com/adafruit/Adafruit_CircuitPython_DS1307 to 2.1.14 from 2.1.13:
  > Merge pull request adafruit/Adafruit_CircuitPython_DS1307#29 from tekktrik/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_GPS to 3.10.0 from 3.9.10:
  > Merge pull request adafruit/Adafruit_CircuitPython_GPS#80 from gamblor21/precision
  > Merge pull request adafruit/Adafruit_CircuitPython_GPS#81 from adafruit/sdcardio

Updating https://github.com/adafruit/Adafruit_CircuitPython_NeoTre to 1.1.11 from 1.1.10:
  > Merge pull request adafruit/Adafruit_CircuitPython_NeoTre#19 from caternuson/iss18

Updating https://github.com/adafruit/Adafruit_CircuitPython_Seesaw to 1.10.12 from 1.10.11:
  > Merge pull request adafruit/Adafruit_CircuitPython_seesaw#104 from ladyada/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_VC0706 to 6.0.4 from 6.0.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_VC0706#31 from tekktrik/doc/add-motion-example
  > Merge pull request adafruit/Adafruit_CircuitPython_VC0706#30 from adafruit/sdcardio

Updating https://github.com/adafruit/Adafruit_CircuitPython_Colorsys to 2.0.8 from 2.0.7:
  > Merge pull request adafruit/Adafruit_CircuitPython_Colorsys#21 from tekktrik/doc/add-missing-params

Updating https://github.com/adafruit/Adafruit_CircuitPython_Display_Text to 2.22.7 from 2.22.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_Display_Text#173 from tekktrik/doc/use-font-protocol

Updating https://github.com/adafruit/Adafruit_CircuitPython_Logging to 4.1.0 from 4.0.2:
  > Merge pull request adafruit/Adafruit_CircuitPython_Logging#31 from tekktrik/dev/publicize-handler-methods

Updating https://github.com/adafruit/Adafruit_CircuitPython_NeoKey to 1.0.8 from 1.0.7:
  > Merge pull request adafruit/Adafruit_CircuitPython_NeoKey#8 from ladyada/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_PYOA to 2.5.6 from 2.5.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_PYOA#33 from adafruit/sdcardio
@AMInnovationTeam AMInnovationTeam mentioned this pull request Apr 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dhalbert dhalbert dhalbert approved these changes

@caternuson caternuson Awaiting requested review from caternuson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gamblor21 @caternuson @dhalbert