-
Notifications
You must be signed in to change notification settings - Fork 934
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
Rhumb line modules #728
Rhumb line modules #728
Conversation
…n (requires rhumb-distance)
@DenisCarriere I don't understand why the tests fail, on my computer everything passes... 😞 |
This is the error, I'll have a look at it.
|
@@ -0,0 +1,50 @@ | |||
{ | |||
"name": "@turf/destination", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stebogit Here's your problem 🤦♂️ You've named it @turf/destination
and when lerna boostrap
creates the symbolic links it overwrites the "real" @turf/destination
with the rhumb module. I'll fix this and the tests should pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
@@ -0,0 +1,50 @@ | |||
{ | |||
"name": "@turf/destination", | |||
"version": "4.3.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For new modules, define them as 4.0.0
(base number of major release) because whenever I publish with Lerna it will automatically bump up the appropriate version (using the same version number as the next release will cause a version conflict with Lerna).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it 👍
packages/turf-rhumb-bearing/index.js
Outdated
@@ -0,0 +1,60 @@ | |||
// https://en.wikipedia.org/wiki/Rhumb_line | |||
// http://www.movable-type.co.uk/scripts/latlong.html#rhumblines | |||
var getCoords = require('@turf/invariant').getCoords; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you're only dealing with points you can use getCoord
which has some extra point validation built-in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't understood the difference between the two yet... Maybe some additional explanation on the doc would help?
I'll use getCoord anyway 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes more documentation is always better 👍
The Gist is getCoord
if you want to retrieve a Point Array (with point validation), getCoords
for the entire coordinates as an Array (limited validation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why then not renaming them getCoords
and getCoordsArray
(I guess this would be suitable for LineString
, Polygon
, etc. right?); I believe that would make it more clear and intuitive.
packages/turf-rhumb-bearing/index.js
Outdated
* | ||
* @name rhumbBearing | ||
* @param {Feature<Point>} start starting Point | ||
* @param {Feature<Point>} end ending Point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth adding Geometry Object
& Array<number>
as valid input (input type supported when using getCoord()
).
* @param {Geometry|Feature<Point>|Array<number>} start starting Point
* origin Point with the (constant) given bearing. | ||
* | ||
* @name rhumbDestination | ||
* @param {Point|Array<number>} origin starting point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same JSDocs as above Geometry|Feature<Point>|Array<number>
* } | ||
* }; | ||
* var distance = 50; | ||
* var bearing = 90;// function (from, distance, bearing, units) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// function (from, distance, bearing, units) {
?
|
||
units = units || 'kilometers'; | ||
|
||
var distanceInMeters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth having distanceInMeters
as a method in @turf/helpers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I was thinking about it; maybe distanceInKm
(just * 1000 to have meters) as that is Turf
's default in many cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes many libraries use meters as the unit of measurement.
I solved this using:
var distanceInMeters = radiansToDistance(distanceToRadians(distance, units), 'meters');
Might be worth having a method that wraps this behavior. Doesn't have to be to Meters it can be to any distance unit type.
convertDistance(1000, 'miles', 'meters')
I dunno about the method name, but something like that.
if (!origin) throw new Error('origin is required'); | ||
if (distance === undefined || distance === null) throw new Error('distance is required'); | ||
if (bearing === undefined || bearing === null) throw new Error('bearing is required'); | ||
if (!(distance >= 0)) throw new Error('distance must be greater than 0'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this if statement from (distance < 0)
. If your input is text, then the result would be false
since text cannot be less than 0.
> -4 < 0
true
> 5 < 0
false
> 'miles' < 0
false
|
||
units = units || 'kilometers'; | ||
|
||
var distanceInMeters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire switch statement can be replaced with only:
var distanceInMeters = radiansToDistance(distanceToRadians(distance, units), 'meters');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very simple and elegant! 👍 👍
if (process.env.REGEN) write.sync(directories.out + name +'.json', result); | ||
t.deepEqual(load.sync(directories.out + name + '.json'), result, name); | ||
|
||
// TODO adopt the following graphical output once rhumbDestination is published |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Sounds like a plan!
packages/turf-rhumb-distance/test.js
Outdated
t.deepEqual(distances, load.sync(directories.out + name + '.json'), name); | ||
|
||
// Removed since (point1 = true & point2 = false) | ||
// t.ok(distances.kilometers < distances.greatCircleDistance, name + ' distance comparison'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stebogit Can you have a look at this failing test case, I don't know if the GreatCircle distance is always greater than Rhumb Line Distance (I would assume that your test statement is correct, however the output says otherwise)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The correct test is reverse, as the GreatCircle line is the shortest path:
distances.kilometers < distances.greatCircleDistance
However it still fails now because of the approximation error due to the incorrect earth radius. I ran the corrected test with 6371km radius and it passes for both cases.
@stebogit Ok I'm done with commits 🤓 Have a glance at it again and give me a 👍 if you're good to merge this, looks good to me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks great!
Looks good to me 😃 |
Added
@turf/rhumb-bearing
@turf/rhumb-destination
@turf/rhumb-distance
Solves #714 and #681
Ref: #684