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

Adds round(), radians2degrees() and degrees2radians() methods to @turf/helpers #715

Merged
merged 8 commits into from
May 8, 2017

Conversation

stebogit
Copy link
Collaborator

@stebogit stebogit commented May 6, 2017

  • Adds radians2degrees and degrees2radians to turf-helpers;
  • Modified precision of degrees conversion reducing Earth radius to it's average value

added angle conversion;
added few tests;
@DenisCarriere
Copy link
Member

DenisCarriere commented May 6, 2017

To be further discussed

  • Modifying Earth Radius, similar discussion here Update distance measurement factors #635 (comment) (also such change would require a major/minor release). Need a 2nd opinion on this change. (need to open a PR for just that change)
  • Remove round() to methods (unnecessary processing?). Edit: At 55M ops/sec I think this is ok 👍

Worth including 👍

  • radians2degrees method (without round)
  • degrees2radians method (without round)
  • round can be included in @turf/helpers. Looks to be a useful method and a modified version has already been used in @turf/truncate - however the multiplier/factor was calculated prior to doing any iterations.

@DenisCarriere
Copy link
Member

DenisCarriere commented May 7, 2017

@stebogit Oops, I thought you did more modifications to the factor. Just realized that you've only changed it from 6373km to 6371km which lines up with what @mourner was discussing in the previous thread.

👍 This PR. Not to sure if this should be deemed a minor release or major release since this will affect a lot of modules.

@stebogit
Copy link
Collaborator Author

stebogit commented May 7, 2017

@DenisCarriere Why don't we add an optional parameter earthRadius that would default to 6371km, which I agree with @mourner is more correct, or any other value, and the user could decide what value to use.
This way Turf could even land on Mars! 😃

@DenisCarriere
Copy link
Member

@mourner's comment does reflect your change, he was mentioning we should use the mean radius which is 6371km, currently TurfJS is using 6373km (dates back over a year ago)

@DenisCarriere
Copy link
Member

DenisCarriere commented May 8, 2017

@stebogit Going to implement radians2degrees, round & degrees2radians, however going to leave the earthRadiusIn/factor changes for another PR (a lot of changes need to happen in dependent modules).

@DenisCarriere
Copy link
Member

DenisCarriere commented May 8, 2017

Merging, build passes on master branch (PR's branch fails on newly updated @turf/buffer)

@DenisCarriere DenisCarriere merged commit a72f715 into master May 8, 2017
@DenisCarriere DenisCarriere deleted the helpers-addition branch May 8, 2017 21:20
@DenisCarriere DenisCarriere self-assigned this May 8, 2017
@mourner
Copy link
Contributor

mourner commented May 11, 2017

Why don't we add an optional parameter earthRadius [...], and the user could decide what value to use. This way Turf could even land on Mars! 😃

I don't think there's a need. Distance calculations depend on radius linearly, meaning that for any calculated value using Earth radius you can do distance * mars_radius / earth_radius to adjust the distance value to Mars.

@DenisCarriere DenisCarriere changed the title angle conversion helper functions, modified precision of conversion Adds radians2degrees and degrees2radians to turf-helpers May 12, 2017
@DenisCarriere DenisCarriere changed the title Adds radians2degrees and degrees2radians to turf-helpers Adds round(), radians2degrees() and degrees2radians() methods to @turf/helpers May 12, 2017
@DenisCarriere
Copy link
Member

@mourner Can you confirm/approve the earth radius change (6373km => 6371km)

Should only be implemented in next major release (v5.0)

Ref: #635 (comment) #635 (comment)
CC: @morganherlocker

@mourner
Copy link
Contributor

mourner commented May 12, 2017

@DenisCarriere I don't know where 6373 comes from, but the mean radius is 6371: https://en.wikipedia.org/wiki/Earth_radius

@DenisCarriere
Copy link
Member

DenisCarriere commented May 12, 2017

@mourner Agreed, seems like 6373km has been in TurfJS since the beginning, I didn't catch this when I reverted the last earth radius commit since I only I reverted it back to its original state.

https://github.com/Turfjs/turf/blame/c81fc99576a37c38af91527462fd812b6c2bc1d2/packages/turf-helpers/index.js#L273

We will go ahead and change it to 6371km in the next major release.
CC: @stebogit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants