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

Feature Request: Support for FeatureCollection<MultiPoint> to first parameter of pointsWithinPolygon #2099

Closed
justinbangerter opened this issue Jun 16, 2021 · 8 comments

Comments

@justinbangerter
Copy link

  • [version 5.1.6 and 6.3.0]
const geojson = turf.featureCollection([
  turf.multiPoint([
    [10, 10],
    [-10, -10]
  ])
]);

const polygon = turf.polygon([[
  [0, 0],
  [15, 0],
  [15, 15],
  [0, 15],
  [0,0]
]]);

turf.pointsWithinPolygon(geojson, polygon);

> "Error: coord must be GeoJSON Point or an Array of numbers"

Thanks for the awesome library.

@rowanwins
Copy link
Member

Good suggestion thanks @justinbangerter !

@rowanwins
Copy link
Member

@micaminoff or @twelch - you've both expressed interest in helping out with TurfJS recently, this might good be a good first PR to tackle :)

@twelch
Copy link
Collaborator

twelch commented Jun 28, 2021

I can take this on. I'd like some feedback on the expected result. @justinbangerter what were you hoping for?

Here's the current docs:

@param {Feature|FeatureCollection<Point|MultiPoint>} points Points as input search
@param {FeatureCollection|Geometry|Feature<Polygon|MultiPolygon>} polygons Points must be within these (Multi)Polygon(s)
@returns {FeatureCollection<Point>} points that land within at least one polygon

So the points argument would be expanded to include MultiPoint.

@param {Feature|FeatureCollection<Point|MultiPoint>} points Points as input search

The return value is more interesting depending on what you think the spirit of the function is, and whether the MultiPoint structure should be maintained or altered. For each MultiPoint input the function could:

  1. break the MultiPoint into individual Point features and return only the ones that fall within a Polygon.
  2. return the whole MultiPoint if any of its point coordinates fall within a Polygon
  3. return a MultiPoint with just the point coordinates that fall within a Polygon

I think the user can/should achieve 1 with the existing function by doing a flatten first to get a collection of Points. By that logic I'm thinking there's only value in supporting MultiPoint for this function if the MultiPoint is maintained (2 or 3).

@justinbangerter
Copy link
Author

justinbangerter commented Jun 28, 2021

I will trust your judgement here. My opinion is that 3 is the best. Or, if you wanted to keep the return type the same, that it would be best to return a collection of points.

The need for this came from having a large geojson with MultiPoint features that I would like to filter in order to get a count of the points inside of the polygon. This worked fine with collections of Point features, which we (where I work) were using due to our need to use some extra metadata attached to the points. When we don't need that metadata, we use MultiPoint features, because they are smaller files.

If you went with option 1, it would seem like a lot of unnecessary, extra processing to transform the object before passing it into the function, because the collection could be flattened afterward, too.

I would think that, if you went for option 2, a more suitable name for the function might be "featuresWithinPolygon."

@twelch
Copy link
Collaborator

twelch commented Jun 28, 2021

@justinbangerter I appreciate the feedback.

I do like 3. It's saying we want to input and return MultiPoint(s), but check and filter out points within them. This keeps with the original description of "Finds Points that fall within (Multi)Polygon(s)".

Interestingly, solution 2 I think is as simple as adding an additional coordEach to the innerLoop. I'll start there and write some tests. Then build on it.

In the meantime, feedback from others welcome.

@justinbangerter
Copy link
Author

Great. Yeah, I think that, if option 2 were implemented, it would not help me accomplish what I wanted to accomplish, which is: find the count of points in the polygon.

@twelch
Copy link
Collaborator

twelch commented Jun 29, 2021

PR is ready for review, implements option 3

@twelch
Copy link
Collaborator

twelch commented Jul 7, 2021

This landed in master, not yet published

@twelch twelch closed this as completed Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants