@turf/booleanCrosses #796

rowanwins · 2017-06-17T13:55:55Z

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.
Overview description of proposed module.
Include JSDocs with a basic example.
Execute ./scripts/generate-readmes to create README.md.
Add yourself to contributors in package.json using "Full Name <@github Username>".

Well I think this is hopefully almost done.

There are 20+ tests (although realistically I think we could still do more!)
I've run the tests against shapely and confirmed the same results
The benchmark results are looking not too bad although I think our line intersections algorithm could be improved
I suggest we make the name booleanCrosses, as 'crosses' (rather than cross) is the name of the predicate everywhere else

 * Benchmark Results
 * LineDoesNotCrossButTouches x 71,945 ops/sec ±2.04% (71 runs sampled)
 * LineDoesNotCrossLine x 88,084 ops/sec ±2.56% (70 runs sampled)
 * LineDoesNotCrossPolygon x 86,024 ops/sec ±2.80% (71 runs sampled)
 * MultiPointNotCrossLine x 11,976,750 ops/sec ±1.61% (93 runs sampled)
 * MultiPointNotCrossLineEnd x 10,191,949 ops/sec ±3.51% (85 runs sampled)
 * LineCrossesLine x 68,764 ops/sec ±2.53% (72 runs sampled)
 * LineCrossesPolygon x 49,268 ops/sec ±2.70% (80 runs sampled)
 * LineCrossesPolygonPartial x 63,313 ops/sec ±2.87% (71 runs sampled)

DenisCarriere · 2017-06-17T14:26:35Z

packages/turf-boolean-crosses/index.js

+        }
+        throw new Error('feature2 ' + type2 + ' geometry not supported');
+    default:
+        throw new Error('feature1 ' + type1 + ' geometry not supported');


type1 cannot be Point/Polygon/MultiLineString? or you are still working on this?

Crosses only supports a very limited set of types, see this link. So as far as I'm concerned I think they're all covered off

DenisCarriere · 2017-06-17T14:27:57Z

👍 that's great that we've got Shapely validation, do you have that code on GitHub anywhere?

Found it: https://gist.github.com/rowanwins/e23208be1c5b27c1af2abdffab546444

dpmcmlxxvi · 2017-06-17T17:18:56Z

packages/turf-boolean-crosses/index.js

+            if (i2 === 0 || i2 === lineString.coordinates.length - 2) {
+                incEndVertices = false;
+            }
+            if (isPointOnLineSegment(lineString.coordinates[i2], lineString.coordinates[i2 + 1], multiPoint.coordinates[i], incEndVertices)) {


Is this excluding both end points of both end segments? Shouldn't it just be the actual LineString end points?

ooo yeah that is a good pickup, a slight modification is needed on the isPointOnLineSegment function

actually now I remember why I did this, I worked out that this didn't matter as the actual 2nd vertex get's picked up in the second segment.

dpmcmlxxvi · 2017-06-17T17:27:29Z

packages/turf-boolean-crosses/index.js

+                if (i2 === 0 || i2 === lineString2.coordinates.length - 2) {
+                    incEndVertices = false;
+                }
+                if (isPointOnLineSegment(lineString1.coordinates[i], lineString1.coordinates[i + 1], lineString2.coordinates[i2], incEndVertices)) {


Why are you checking that the line segment 1 end points fall on line segment 2? cross only requires that they share some but not all interior points and that the intersections be points. They don't have to be the vertex points.

ah ok so I think here Im just checking that the point of intersection (or one of them) occurs somewhere other than the very end of the line.

rowanwins · 2017-06-18T10:01:11Z

Gday @dpmcmlxxvi

Thanks for the review. Perhaps the best thing to do is if you've got a concern is perhaps add some more test cases and we'll see how they come out.

dpmcmlxxvi · 2017-06-18T17:24:10Z

@rowanwins Will do. It maybe a little bit before I get some time to work on it but will work on putting some tests together.

DenisCarriere · 2017-06-18T17:32:36Z

@rowanwins @dpmcmlxxvi As for the types, shouldn't the opposite input types also be valid?

Feature1 => Feature2

LineString => Polygon
Polygon => LineString (Currently throws an error)
MultiPoint => Polygon
Polygon => MultiPoint (Currently throws an error)
LineString => Polygon
Polygon => LineString (Currently throws an error)
Other combinations

ST_Crosses returns 1 or t (TRUE) if the intersection results in a geometry that has a dimension that is one less than the maximum dimension of the two source geometries and the intersection set is interior to both source geometries. ST_Crosses returns 1 or t (TRUE) for only ST_MultiPoint/ST_Polygon, ST_MultiPoint/ST_LineString, ST_Linestring/ST_LineString, ST_LineString/ST_Polygon, and ST_LineString/ST_MultiPolygon comparisons.

Question: Shouldn't we return False instead of throwing an Error?

dpmcmlxxvi · 2017-06-18T17:56:03Z

Agreed that the opposite input types are valid and should be allowed. Should be easy enough to implement. Use same functions just flip the geom# arguments.

On the return value. I can go either way. I do think there is some value to the way @rowanwins has it now. It might be helpful to make a distinction between having valid vs. invalid inputs. Returning False for DE-9IM functions that are not defined for certain inputs types might be misleading.

DenisCarriere · 2017-06-18T18:00:06Z

I think if we include the reverse inputs we will have very little "invalid" input types.

Ex: Point => Point - should be invalid since two points cannot cross each other.

DenisCarriere · 2017-06-18T18:01:23Z

@rowanwins FYI: Going to include your shapely script into /scripts

rowanwins · 2017-06-19T10:41:47Z

Gday @DenisCarriere

Good thinking, it looks like the inverse can be covered in crosses ( I guess crosses is a bit different from of the other booleans in that regard), I'll include those in.

DenisCarriere · 2017-06-19T13:29:08Z

@rowanwins Wow, this actually worked, I was able to include the shapely tests directly in the Tape tests.

glob.sync(path.join(__dirname, 'test', 'true', '*.geojson')).forEach(filepath => {
    const {name} = path.parse(filepath);
    const geojson = load.sync(filepath);
    const [feature1, feature2] = geojson.features;
    if (process.env.SHAPELY) shapely('crosses', feature1, feature2).then(result => t.true(result, '[true] shapely - ' + name));
    t.true(crosses(feature1, feature2), '[true] ' + name);
});

Just make sure you define your environment variable as SHAPELY=true and you have Python + shapely installed.

DenisCarriere

👍 Looks good

DenisCarriere · 2017-06-19T13:59:51Z

Merging, all the tests pass (including Shapely results).

We can always improve this library by adding more test fixtures, so far it looks good to me.

rowanwins added 2 commits June 17, 2017 23:50

Initial commit of boolean-crosses

7727483

Doh, didn't mean to commit those :(

7b8cf44

DenisCarriere reviewed Jun 17, 2017

View reviewed changes

dpmcmlxxvi reviewed Jun 17, 2017

View reviewed changes

DenisCarriere assigned rowanwins Jun 18, 2017

rowanwins and others added 3 commits June 19, 2017 20:54

Add support for inverse operations

5d84aaf

Merge branch 'master' into boolean-crosses

a7b95b0

Include shapely scripts in testing

61d81bc

DenisCarriere added this to the 4.5.0 milestone Jun 19, 2017

DenisCarriere added the new-module label Jun 19, 2017

DenisCarriere approved these changes Jun 19, 2017

View reviewed changes

Add python-shell to devDependencies

2b62e73

DenisCarriere merged commit 28f6dcd into master Jun 19, 2017

DenisCarriere deleted the boolean-crosses branch June 19, 2017 14:24

This was referenced Jun 27, 2017

Not found : @turf/boolean-crosses #818

Closed

New minor release! Turf 4.5.2 🎉 #826

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@turf/booleanCrosses #796

@turf/booleanCrosses #796

rowanwins commented Jun 17, 2017

DenisCarriere Jun 17, 2017

rowanwins Jun 18, 2017

DenisCarriere commented Jun 17, 2017 •

edited

Loading

dpmcmlxxvi Jun 17, 2017

rowanwins Jun 18, 2017

rowanwins Jun 18, 2017

dpmcmlxxvi Jun 17, 2017

rowanwins Jun 18, 2017 •

edited

Loading

rowanwins commented Jun 18, 2017

dpmcmlxxvi commented Jun 18, 2017 •

edited

Loading

DenisCarriere commented Jun 18, 2017 •

edited

Loading

dpmcmlxxvi commented Jun 18, 2017

DenisCarriere commented Jun 18, 2017

DenisCarriere commented Jun 18, 2017

rowanwins commented Jun 19, 2017

DenisCarriere commented Jun 19, 2017

DenisCarriere left a comment

DenisCarriere commented Jun 19, 2017

@turf/booleanCrosses #796

@turf/booleanCrosses #796

Conversation

rowanwins commented Jun 17, 2017

DenisCarriere Jun 17, 2017

Choose a reason for hiding this comment

rowanwins Jun 18, 2017

Choose a reason for hiding this comment

DenisCarriere commented Jun 17, 2017 • edited Loading

dpmcmlxxvi Jun 17, 2017

Choose a reason for hiding this comment

rowanwins Jun 18, 2017

Choose a reason for hiding this comment

rowanwins Jun 18, 2017

Choose a reason for hiding this comment

dpmcmlxxvi Jun 17, 2017

Choose a reason for hiding this comment

rowanwins Jun 18, 2017 • edited Loading

Choose a reason for hiding this comment

rowanwins commented Jun 18, 2017

dpmcmlxxvi commented Jun 18, 2017 • edited Loading

DenisCarriere commented Jun 18, 2017 • edited Loading

dpmcmlxxvi commented Jun 18, 2017

DenisCarriere commented Jun 18, 2017

DenisCarriere commented Jun 18, 2017

rowanwins commented Jun 19, 2017

DenisCarriere commented Jun 19, 2017

DenisCarriere left a comment

Choose a reason for hiding this comment

DenisCarriere commented Jun 19, 2017

DenisCarriere commented Jun 17, 2017 •

edited

Loading

rowanwins Jun 18, 2017 •

edited

Loading

dpmcmlxxvi commented Jun 18, 2017 •

edited

Loading

DenisCarriere commented Jun 18, 2017 •

edited

Loading