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

Adding combine turf method #1008

Merged
merged 1 commit into from
Sep 25, 2019
Merged

Adding combine turf method #1008

merged 1 commit into from
Sep 25, 2019

Conversation

langsmith
Copy link

This pr ports the combine method to Turf for Java. This method takes a FeatureCollection of Point, LineString, or Polygon features and returns a Geometry. The returned Geometry can be used to create a MultiPoint, MultiLineString, or MultiPolygon feature.

http://turfjs.org/docs/#combine

https://github.com/Turfjs/turf/blob/master/packages/turf-combine/index.ts

combine(FeatureCollection featureCollection) is the new method added to the TurfConversion class.

Screen Shot 2019-04-19 at 9 05 10 PM

Screen Shot 2019-04-20 at 6 53 10 AM

@langsmith
Copy link
Author

The returned Geometry can be used to create a MultiPoint, MultiLineString, or MultiPolygon feature.

For example,

 MultiLineString newMultiLineStringObject = (MultiLineString) TurfConversion.combine(lineStringFeatureCollection);

@langsmith langsmith force-pushed the ls-adding-combine-turf-method branch from 3d97fab to 7de3123 Compare April 21, 2019 02:09
@langsmith langsmith force-pushed the ls-adding-combine-turf-method branch 2 times, most recently from 75a28eb to f59f7e6 Compare April 25, 2019 13:34
@osana osana added this to the v4.8.0 milestone May 2, 2019
@langsmith langsmith force-pushed the ls-adding-combine-turf-method branch from f59f7e6 to 9e4cf36 Compare May 2, 2019 18:21
@osana osana modified the milestones: v4.8.0, v4.9.0 May 7, 2019
@langsmith langsmith force-pushed the ls-adding-combine-turf-method branch 7 times, most recently from ba1cf30 to 730297d Compare May 20, 2019 22:21
@langsmith langsmith force-pushed the ls-adding-combine-turf-method branch from 730297d to 9ec9092 Compare June 4, 2019 21:21
@langsmith langsmith force-pushed the ls-adding-combine-turf-method branch 2 times, most recently from 44c9d07 to 28b2d4a Compare June 5, 2019 17:29
@langsmith
Copy link
Author

Return type should be a FeatureCollection

Fixed

@langsmith langsmith force-pushed the ls-adding-combine-turf-method branch 3 times, most recently from ce3e28e to ff773eb Compare June 5, 2019 19:35
@langsmith langsmith force-pushed the ls-adding-combine-turf-method branch from c3524ee to c1cfb88 Compare June 6, 2019 00:22
@langsmith
Copy link
Author

Ok, FeatureCollection is now returned in all scenarios and I pushed some refactoring which checks that all geometries are of the same type. If not,

thrown.expectMessage(startsWith("Your FeatureCollection must be of all of the same geometry type."));
is thrown.

@langsmith langsmith self-assigned this Jun 6, 2019
@langsmith
Copy link
Author

👉👉@tobrun

@langsmith langsmith force-pushed the ls-adding-combine-turf-method branch from c1cfb88 to 8a689c9 Compare June 14, 2019 18:48
@langsmith langsmith force-pushed the ls-adding-combine-turf-method branch 2 times, most recently from 7bd27f1 to b3b2013 Compare July 10, 2019 23:32
@langsmith
Copy link
Author

Ok @tobrun . Pushed some changes and responded to comments above. Would love another round of eyes when you get a chance 🙇

@langsmith langsmith requested a review from LukasPaczos August 6, 2019 20:18
@langsmith langsmith force-pushed the ls-adding-combine-turf-method branch from b3b2013 to f513c43 Compare September 6, 2019 20:39
@langsmith
Copy link
Author

👉👉 @tobrun 😬

@tobrun
Copy link
Member

tobrun commented Sep 9, 2019

A single FeatureCollection is the parameter. The FeatureCollection has to have Point and/or MultiPoint, LineString and/or MultiLineString, or Polygon and/or MultiPolygon. No mixing Point-based geometries with Polygon-based, etc.

The heterogeneous test showcase that is should be possible to combine both Point with Polygon. To get the proper implementation on the java side, I should try fully porting the testsuite on the js implementation. This helps to verify assumptions you are making about the code.

@langsmith
Copy link
Author

The heterogeneous test showcase that is should be possible to combine both Point with Polygon. To get the proper implementation on the java side, I should try fully porting the testsuite on the js implementation. This helps to verify assumptions you are making about the code.

I'd missed the inclusion of the Point at https://github.com/Turfjs/turf/blob/master/packages/turf-combine/test.js#L188 🔬 . Confirmed by getting more clarification at Turfjs/turf#1756.

Refactored the method implementation and tests so that heterogenous combinations are allowed. Rebased. The order of my tests in this pr now match the test order in https://github.com/Turfjs/turf/blob/master/packages/turf-combine/test.js. While the coordinates differ, my tests check the same situations as in https://github.com/Turfjs/turf/blob/master/packages/turf-combine/test.js (e.g. Point & Point, LineString & MultiLineString, etc). I also added combinePointAndMultiPolygonAndLineStringGeometry, combinePointAndLineStringGeometry, and combine_featureCollectionSizeCheck.

Would love another round of 👀 when ya'll get a chance.

@langsmith langsmith force-pushed the ls-adding-combine-turf-method branch from 071a8d5 to 4c40700 Compare September 23, 2019 22:13
@langsmith langsmith force-pushed the ls-adding-combine-turf-method branch from 4c40700 to b976805 Compare September 25, 2019 16:18
@langsmith langsmith merged commit 4229ef0 into master Sep 25, 2019
@langsmith langsmith deleted the ls-adding-combine-turf-method branch September 25, 2019 18:09
@langsmith langsmith modified the milestones: v4.9.0, 4.10.0 Sep 25, 2019
@langsmith langsmith mentioned this pull request Jan 8, 2020
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants