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

Feedback: why I'm using turf-inside of polygonContainsPoint #273

Closed
markstos opened this issue Apr 22, 2016 · 4 comments
Assignees

Comments

@markstos
Copy link
Contributor

@markstos markstos commented Apr 22, 2016

I was going to use this library to test if a point was contained within a
polygon, but I ran into a number of hang-ups. Then I found Turf.js had a
comparable function which I was able to correctly test on my first try. Here are the challenges I ran into.

  • There's no example of how to call a method from Terraformer.Tools. I guessed it would work like the Circle: var tools = new Terraformer.Tools(), but that didn't work. I resorted to searching the source code to find an example in the test suite.
  • For coordinatesContainsPoint, it not clear whether the definition of "contains" is in the mathematical sense-- a literal matchin the data structure-- or in the geographic sense.
  • The link to "Polygon" in the docs is broken. it should go to #polygon, not #geojson-polygon
  • The documentation for "Polygon" is incomplete and wrong. The current definition is simply: "An array of coordinates defining a polygon. This definition doesn't even match the example, which shows an extra level of nesting. It appears that the GeoJSON definition is intended. It's clear why that definition wasn't just copy/pasted.
  • It's not clear the data structure to use is really just the "coordinates" value form the GeoJSON structure or the fully GeoJSON polygon structure. The docs show a full structure, but the code I finally got to work used just the "coodinates" value. These should match. If both data structures are accepted, that should be explained.
  • There's no validation on the input. Instead "false" is returned on bad input, leaving me to wonder if my data structures were bad, the code was broken or the calculation was truly false. Lack of validation could possibily be justifified for performance, but it made it hard to get start with.
  • There is no test coverage for polygonContainsPoint actually working. Only some failure cases are covered. So, I couldn't even find a working example in the test and was left to wonder if the code actually /could/ work.

Finally, I want to create a pull request to improve some of these things, and "npm test" doesn't even pass for me /before/ I make any changes (with Node.js 4.4 / Ubuntu ). At that point I gave up.

After that, I found and tried turf-inside.

It includes clear examples both textually and visually for a success and failure case. It was very easy to use that to construct some of my own test cases of success and failure.

http://turfjs.org/static/docs/module-turf_inside.html

It also has great test coverage:

https://github.com/Turfjs/turf-inside/blob/master/test.js

I think now I appreciate the recent proposal for dramatic refactoring:
#268

After this experience, I would have to agree that focusing on the Wkt and ArcGIS parsers make sense, as it seems that Turf is already doing a great job where the feature set overlaps with Terraformer.

@jgravois

This comment has been minimized.

Copy link
Contributor

@jgravois jgravois commented Apr 22, 2016

wow. thank you so much for the comprehensive writeup.

i'm especially frustrated to hear that the tests aren't passing for you on master because i just fixed them on my side (in #265, tested on El Cap, Node.js 4.4.3).

what exactly do you see?

@markstos

This comment has been minimized.

Copy link
Contributor Author

@markstos markstos commented Apr 22, 2016

@jgravois I just a fresh git clone / npm install /npm test in the same environment, and the tests pass. So I'm sure what the issue in the copy of my tree is-- sorry for the false alarm there.

On the plus side for Terraformer, you have a method that allows for generating a circle-shaped polygon with a specified number of sides. turf-buffer also does something like that, but it doesn't yet allow you to specify the resolution of the circle like Terraformer does.

@jgravois jgravois self-assigned this Apr 22, 2016
@jgravois

This comment has been minimized.

Copy link
Contributor

@jgravois jgravois commented Apr 22, 2016

good to hear.

<edit to earlier incorrect statement>

the problem you encountered is the result of the fact that the method expects you to pass the coordinate array (or nested coordinate array) of your GeoJSON polygon geometry directly.

var Terraformer = require('terraformer');

var pt = [-111.467285, 40.75766];
var pt2 = [-111.873779, 40.647303];

var polygon = {
  "type": "Polygon",
  "coordinates": [[
    [-112.074279, 40.52215],
    [-112.074279, 40.853293],
    [-111.610107, 40.853293],
    [-111.610107, 40.52215],
    [-112.074279, 40.52215]
  ]]
};

var polygonGeometry = polygon.coordinates;

Terraformer.Tools.polygonContainsPoint(polygonGeometry, pt); // returns false
Terraformer.Tools.polygonContainsPoint(polygonGeometry, pt2); // returns true

as you said, the fact that we had no test coverage to confirm that the function worked was a serious omission. additionally, there were several problems in the Terraformer documentation, so your confusion was entirely warranted.

</edit to earlier incorrect statement>

@jgravois

This comment has been minimized.

Copy link
Contributor

@jgravois jgravois commented Jun 24, 2016

i pushed a sample to the doc page to help other folks in the future, so i'm going to go ahead and close this issue.

thanks again for the feedback.

@jgravois jgravois closed this Jun 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.