Clarification about @turf-angle#1192
Conversation
@stebogit I agree with all your comments, this was more a "draft" module, it's still not implemented in |
|
@stebogit I implemented your Have a look at it to see if that's what you were thinking. |
DenisCarriere
left a comment
There was a problem hiding this comment.
@stebogit 👍 Looks good to merge
|
@stebogit I've added some color the test outputs, a lot easier to visualize the outputs now. Obtuse Angle
Acute Angle
|
|
@DenisCarriere I tried to add a short description in the jdocs, but I believe it can be explained better. I also added few tests. This looks ok to me, you did all the work! 😄 👍 |
You did the initial work 👍 😄 Good stuff, i'll merge |
|
|
||
| // Explementary angle | ||
| if (explementary === true) return 360 - angleAO; | ||
| if (options.explementary === true) return 360 - angleAO; |
There was a problem hiding this comment.
👍 I'm also starting to like using options.* instead of defining them at the beginning, it's easier to tell which variables are derived from the options.
packages/turf-angle/test.js
Outdated
| const pt1 = point([-10, -30]); | ||
| const pt2 = point([-11, -33]); | ||
| const pt3 = point([-12, -36]); | ||
| t.throws(() => angle(null, pt2, pt3), /obj is required/, 'missing startPoint'); |
There was a problem hiding this comment.
Maybe we should throw an Error that says missing startPoint instead of obj is required
There was a problem hiding this comment.
Yes, I was concerned about the performance, but it should not be a big deal.
Have you checked the benchmark by any chance?
There was a problem hiding this comment.
I wouldn't worry about performance for those types of checks, it doesn't slow anything down.
The biggest performance hits is reiterating over a FeatureCollection twice when 1 pass can achieve the same result.
I'd say all the input type checking is always valid to add, the "heavy" performance hit will be the main logic and not the input checking.
Currently the new
@turf-angleis calculating the angle defined by three points (A,oandBin the picture) is defined/calculated as the angle (< 180˚) between the lines extendingAoandoB; as a result the anglesAoBandBoAare equals.Note: passing the option
exterior=truewill return the explementary angle (360 -angle).Now, I believe a more useful calculation (maybe in addition to the current one) would be defining the angle as the clockwise (or counterclockwise) angle between
A(startPoint) andB(endPoint) with center ino(midPoint). This would allow comparison between angles due to the referenceAosegment.In the picture above the current implementation of the module would return the red angle for
AoBand the blue dotted angle forAoB'. I believe it would be more useful gettingAoB'as the solid blue line, otherwise there is no way to properly compare the two angles.In this PR I just added a few test cases to confirm and highlight the current behavior.
@DenisCarriere @rowanwins I propose to modify the output as suggested (here a possible implementation, but I'm sure we can find a better one) and adding an option (
absolute?) to provide the current result.I'd also rename the
externaloption toexplementary.If the current output is fine I'd probably add some notes to the docs to clarify the output.