turf/booleanContains

[rowanwins]

• 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>".

Gday @DenisCarriere

Well I think this is hopefully almost done.

• There are 20+ tests (although realistically I think we could still do more!)
• I've put together this script to validate the results against shapely which has proved to be very handy in understanding the interpretation of the predicate rules.
• The benchmark results are looking really good (see below), the bulk are performing well over a 1M ops/second
  • Although you'll notice I've replaced the deep-compare dependency, deep-compare appears to be very slow. For example I went from 176,000 operations to 9,000,000 operations
• I suggest we make the name booleanContains, that way the boolean part is explicit and people won't misinterpret what the module might do

It's taken a while but we're finally getting there :)

 * Benchmark Results
 *
 * LineIsNotContainedByLine x 4,133,850 ops/sec ±1.62% (92 runs sampled)
 * LineIsNotContainedByPolygon x 1,416,083 ops/sec ±2.47% (87 runs sampled)
 * LineIsNotContainedByPolygonBoundary x 1,419,828 ops/sec ±1.85% (88 runs sampled)
 * MultiPointIsNotContainedByMultiPoint x 6,705,712 ops/sec ±4.32% (76 runs sampled)
 * MultiPointIsNotContainedByPolygon x 2,043,151 ops/sec ±3.21% (82 runs sampled)
 * MultiPointsIsNotContainedByLine x 5,210,528 ops/sec ±3.21% (74 runs sampled)
 * PointIsNotContainedByLine x 7,324,482 ops/sec ±4.22% (84 runs sampled)
 * PointIsNotContainedByLineBecauseOnEnd x 6,024,271 ops/sec ±5.04% (78 runs sampled)
 * PointIsNotContainedBYMultiPoint x 10,063,229 ops/sec ±3.50% (80 runs sampled)
 * PointIsNotContainedByPolygon x 2,204,858 ops/sec ±3.03% (80 runs sampled)
 * PointOnPolygonBoundary x 2,387,408 ops/sec ±2.47% (83 runs sampled)
 * PolygonIsNotContainedByPolygon x 1,999,776 ops/sec ±3.20% (84 runs sampled)
 * LineIsContainedByLine x 3,434,699 ops/sec ±3.43% (82 runs sampled)
 * LineIsContainedByPolygon x 853,234 ops/sec ±2.43% (79 runs sampled)
 * MultiPointIsContainedByPolygonBoundary x 1,197,014 ops/sec ±1.89% (85 runs sampled)
 * MultiPointsContainedByMultiPoints x 6,601,619 ops/sec ±2.44% (80 runs sampled)
 * MultipointsIsContainedByLine x 5,843,279 ops/sec ±2.37% (82 runs sampled)
 * PointInsidePolygonBoundary x 2,110,993 ops/sec ±3.84% (79 runs sampled)
 * PointIsContainedByLine x 7,436,720 ops/sec ±2.74% (80 runs sampled)
 * PointIsContainedByMultiPoint x 10,256,333 ops/sec ±4.22% (75 runs sampled)
 * PolygonIsContainedByPolygon x 757,885 ops/sec ±2.61% (82 runs sampled)
 * PolygonsExactSameShape x 758,328 ops/sec ±2.27% (86 runs sampled)

[dpmcmlxxvi]

@rowanwins Is the goal to implement the DE-9IM definitions of these functions? Because this approach is not implementing contains definition. There is no check that any point falls in the interior. This allows all points to fall on boundary.

[rowanwins]

Gday @dpmcmlxxvi . Yep the aim is to match the DE-9IM, good pickup thank you. The sheer amount of scenarios that have to be catered for is huge, Im trying to include as many tests as required to make sure we covered them all off.

[dpmcmlxxvi]

@rowanwins I think this assumes the lines are oriented the same way. Couldn't they be reversed?

[rowanwins]

correct again, the variations are many :)

[rowanwins]

actually on second read of the contains rules I think I can actually ditch this check

The pattern matrix of the ST_Contains predicate states that the interiors of both geometries must intersect and that the interior and boundary of the secondary (geometry b) must not intersect the exterior of the primary (geometry a).

Although in tidying it up I did find another little fix - ah the subtleties :)

[dpmcmlxxvi]

@rowanwins Same issue as above. contains only requires that some line point is in the polygon interior, not necessarily a vertex. You can have a line in which vertices are entirely on the boundary but the line is still contained.

[rowanwins]

Ah yes ok I can see where you're coming from on this one, very thorough reviewing thanks @dpmcmlxxvi :)

[dpmcmlxxvi]

Just wondering why these are being called @turf/booleanXYZ as opposed to just @turf/contains, @turf/crosses, etc? Because they are returning true/false? They could just get grouped in the docs.

[rowanwins]

Gday @dpmcmlxxvi

Thanks for the thorough review! I was including boolean in the name because of

1. There are other modules that will have name conflicts, eg intersects
2. It makes it very explicit as to what the module will return

[rowanwins]

Gday @dpmcmlxxvi & @DenisCarriere

I've rejigged a couple of the checks as per comments from @dpmcmlxxvi . Performance still seems pretty good.

I think the best thing we can to is keep thinking of scenarios and adding them to the tests (both positive and negative results). There are just so many flippin scenarios is hard to think through all of them as you write the code (perhaps I should add some more comments in the code to explain my thought logic).

Anyway let me know if there are any further requests.

[DenisCarriere]

@turf/bbox is repeated in both (dev)dependencies

[DenisCarriere]

👍 All test cases look good to me.