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

Fix location of intersect point for @turf/point-on-line. #750

Merged
merged 1 commit into from
May 23, 2017

Conversation

dpmcmlxxvi
Copy link
Collaborator

This addresses #691 and #722. A few issues to point out:

  • The location property of the intersectPt was being computed using the closestPt which was initialized to Infinity which yielded spurious results. I believe it should be computed using intersectPt.
  • I think the current implementation was passing its tests because the expected fixture values were using the values from the algorithm itself. For example, the test turf-point-on-line - points on top of line uses a line with length 0.30576 and places 10 equally spaced points on it. So the first point should have been at 0.030576 but the test was expecting it at 0.021111. Hence the fixtures have been updated accordingly.

I checked the results and #691 and #722 appear to have been fixed. Could use a double check though.


Please fill in this template.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Have read How To Contribute.
  • Run npm test at the sub modules where changes have occurred.
  • Run npm run lint to ensure code style at the turf module level.

@dpmcmlxxvi dpmcmlxxvi changed the title Fix location of intersect point. Fix location of intersect point for @turf/point-on-line. May 20, 2017
@dpmcmlxxvi dpmcmlxxvi requested review from DenisCarriere and removed request for DenisCarriere May 22, 2017 03:25
@DenisCarriere
Copy link
Member

Thanks, this change makes sense, no more NaN

Copy link
Member

@DenisCarriere DenisCarriere left a comment

Choose a reason for hiding this comment

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

Looks good!

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

Successfully merging this pull request may close these issues.

2 participants