Reimplement coord_pos_relative_to_ring using wn algo #972

bjornharrtell · 2023-01-23T12:21:29Z

Doing this mostly for fun but AFAIK the winding number algo is superior to the ray-intersection algo.

bjornharrtell · 2023-01-23T12:23:34Z

Getting 6 test failures which I'm as of yet mystified about.

bjornharrtell · 2023-01-23T15:51:51Z

Found the problem, was porting error. Then I found that I could use some preexisting bits of code instead.

bjornharrtell · 2023-01-23T19:35:11Z

Looks like this improves performance significantly (~40% !) at least for the intersection bench. My results:

1. time:   [1.4524 s 1.4671 s 1.4823 s]
2. time:   [625.52 ms 626.94 ms 628.31 ms]
3. time:   [896.23 ms 899.61 ms 902.87 ms]

1. before this PR
2. after this PR
3. attempt to rewrite with pattern matching to reduce redundant code (not commited or pushed)

urschrei

Just a minor query from me

urschrei · 2023-01-23T19:47:45Z

geo/src/algorithm/intersects/mod.rs

@@ -100,7 +100,7 @@ where
 // Helper function to check point lies inside rect given by
 // bounds.  The first bound need not be min.
 #[inline]
-fn point_in_rect<T>(value: Coord<T>, bound_1: Coord<T>, bound_2: Coord<T>) -> bool
+pub fn point_in_rect<T>(value: Coord<T>, bound_1: Coord<T>, bound_2: Coord<T>) -> bool


Can this be made pub crate?

doesn't appear so on naive try

See https://github.com/georust/geo/pull/975/files#diff-202b9782bca6975a5e99f3e0264722b95db4f2caf099f8b79f8be015ec947f16R103

michaelkirk

This is really exciting, thanks @bjornhartell!

I'm running some benches locally, because I want to see how it affects other algorithms, like the GeometryGraph stuff, which has a pretty comprehensive test suite (lifted from JTS) and uses Intersects under the hood.

michaelkirk · 2023-01-23T21:17:49Z

geo/src/algorithm/coordinate_position.rs

        }
    }
-    if crossings % 2 == 1 {
+    if wn != 0 {


Clarity nit: it's a tiny bit easier to read this without the negation:

if wn == 0 { CoordPos::Outside } else { CoordPos::Inside }

michaelkirk · 2023-01-23T21:21:01Z

geo/src/algorithm/coordinate_position.rs

-    let mut crossings = 0;
+    // Use winding number algorithm with on boundary short-cicuit
+    // See: https://en.wikipedia.org/wiki/Point_in_polygon#Winding_number_algorithm
+    let mut wn = 0;


Please prefer more explicable names, unless it's extremely local (like a one line closure):

Suggested change

let mut wn = 0;

let mut winding_number = 0;

michaelkirk · 2023-01-24T00:13:37Z

I'm running some benches locally, because I want to see how it affects other algorithms, like the GeometryGraph stuff, which has a pretty comprehensive test suite (lifted from JTS) and uses Intersects under the hood.

As well as a 40-50% improvement for our intersection bench, this has a big speedup for our contains bench!

I did see some regressions across the boolean-ops benches. Is anyone else able to reproduce?:

bench snippet

Circular polygon boolean-ops/bops::intersection/4096
                        time:   [26.771 ms 27.050 ms 27.435 ms]
                        change: [+10.853% +12.476% +14.037%] (p = 0.00 < 0.05)
                        Performance has regressed.
Circular polygon boolean-ops/rgbops::union/4096
                        time:   [13.865 ms 14.073 ms 14.379 ms]
                        change: [+7.4215% +9.4913% +11.382%] (p = 0.00 < 0.05)
                        Performance has regressed.
Circular polygon boolean-ops/rgbops::intersection/4096
                        time:   [18.950 ms 19.027 ms 19.137 ms]
                        change: [+9.4979% +10.451% +11.338%] (p = 0.00 < 0.05)
                        Performance has regressed.
Circular polygon boolean-ops/geo::relate/4096
                        time:   [21.060 ms 21.496 ms 21.757 ms]
                        change: [+5.0324% +7.0601% +8.6649%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 1 outliers among 10 measurements (10.00%)
  1 (10.00%) low mild
Circular polygon boolean-ops/bops::union/8192
                        time:   [29.972 ms 30.165 ms 30.379 ms]
                        change: [+18.804% +20.426% +22.553%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 1 outliers among 10 measurements (10.00%)
  1 (10.00%) high severe
Circular polygon boolean-ops/bops::intersection/8192
                        time:   [57.669 ms 59.629 ms 61.267 ms]
                        change: [+11.755% +14.677% +17.924%] (p = 0.00 < 0.05)
                        Performance has regressed.
Circular polygon boolean-ops/rgbops::union/8192
                        time:   [30.960 ms 31.284 ms 31.638 ms]
                        change: [+4.4674% +6.2686% +8.1893%] (p = 0.00 < 0.05)
                        Performance has regressed.
Circular polygon boolean-ops/rgbops::intersection/8192
                        time:   [37.538 ms 38.036 ms 39.204 ms]
                        change: [+4.2654% +7.4364% +11.293%] (p = 0.00 < 0.05)
                        Performance has regressed.
Circular polygon boolean-ops/geo::relate/8192
                        time:   [67.582 ms 67.820 ms 68.166 ms]
                        change: [+9.9190% +10.866% +11.935%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 1 outliers among 10 measurements (10.00%)
  1 (10.00%) high mild

I'm not sure what accounts for it. Note the bops benchs are currently mislabled ~~and includes some redundancies~~ (edit: no redundancies, just my own confusion). I just now opened #973 to address that and another unrelated fix for the relate bench.

urschrei · 2023-01-24T00:48:35Z

Those regressions are very surprising – I can't think of any obvious explanation. @rmanoka any ideas about what could be going on?

michaelkirk · 2023-01-24T00:56:27Z

 Circular polygon boolean-ops/rgbops::union/8192
                        time:   [30.960 ms 31.284 ms 31.638 ms]
                        change: [+4.4674% +6.2686% +8.1893%] (p = 0.00 < 0.05)
                        Performance has regressed.
Circular polygon boolean-ops/rgbops::intersection/8192
                        time:   [37.538 ms 38.036 ms 39.204 ms]
                        change: [+4.2654% +7.4364% +11.293%] (p = 0.00 < 0.05)
                        Performance has regressed.

I'm actually really skeptical of my results on that bops bench. For example, I think these "regressed" benches are actually testing an old unchanged version of geo-types (via the boolean_ops crates). So I don't even think it should be possible that we could affect that behavior.

Maybe it's just ghosts in the machine... 👻

michaelkirk · 2023-01-24T01:04:41Z

Yeah actually, I don't have any concerns about the boolean ops "regressions" relating to this PR. I think it's just spurious.

I did:

pub fn coord_pos_relative_to_ring<T>(coord: Coord<T>, linestring: &LineString<T>) -> CoordPos
where
    T: GeoNum,
{
+    panic!("in coord_pos_relative_to_ring");

coord_pos_relative_to_ring isn't even called in BoolOps code path from what I can tell.

urschrei · 2023-01-24T01:39:37Z

bors r+

bors · 2023-01-24T01:46:04Z

Build succeeded:

ci result

urschrei · 2023-01-24T01:48:30Z

Ugh I saw the approve and didn't pay attention to the nits. I'll clean those up in the morning.

michaelkirk · 2023-01-24T03:08:40Z

No worries! If it was anything I felt strongly about I wouldn't have marked it as "approved".

michaelkirk · 2023-01-24T17:20:45Z

Ugh I saw the approve and didn't pay attention to the nits. I'll clean those up in the morning.

I did this in #975.

975: fix nits from #972 r=urschrei a=michaelkirk - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/main/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>

bjornharrtell marked this pull request as draft January 23, 2023 12:21

Reimplement coord_pos_relative_to_ring using wn algo

a65ba65

bjornharrtell force-pushed the wn branch from 1524e69 to a65ba65 Compare January 23, 2023 12:23

bjornharrtell added 3 commits January 23, 2023 16:18

Move all to coordinate_position

b5031f5

Fix port error

4cb1e85

Use preexisting impl

b9ad61f

bjornharrtell marked this pull request as ready for review January 23, 2023 15:51

bjornharrtell added 5 commits January 23, 2023 16:52

Remove left over

3b77214

Fix formatting

9cf7a6b

Fix unused import

0749eaf

Fix coord import

31ee71d

Fix formatting

701219e

urschrei approved these changes Jan 23, 2023

View reviewed changes

michaelkirk reviewed Jan 23, 2023

View reviewed changes

michaelkirk approved these changes Jan 24, 2023

View reviewed changes

bors bot merged commit 923ba6a into georust:main Jan 24, 2023

michaelkirk added a commit that referenced this pull request Jan 24, 2023

fix nits for #972

39d6cdd

michaelkirk mentioned this pull request Apr 4, 2023

Performance fix for point in polygon #1004

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement coord_pos_relative_to_ring using wn algo #972

Reimplement coord_pos_relative_to_ring using wn algo #972

bjornharrtell commented Jan 23, 2023 •

edited

Loading

bjornharrtell commented Jan 23, 2023

bjornharrtell commented Jan 23, 2023

bjornharrtell commented Jan 23, 2023 •

edited

Loading

urschrei left a comment

urschrei Jan 23, 2023

bjornharrtell Jan 24, 2023

michaelkirk Jan 24, 2023

michaelkirk left a comment

michaelkirk Jan 23, 2023

michaelkirk Jan 23, 2023

michaelkirk commented Jan 24, 2023 •

edited

Loading

urschrei commented Jan 24, 2023

michaelkirk commented Jan 24, 2023

michaelkirk commented Jan 24, 2023

urschrei commented Jan 24, 2023

bors bot commented Jan 24, 2023

urschrei commented Jan 24, 2023

michaelkirk commented Jan 24, 2023

michaelkirk commented Jan 24, 2023

Reimplement coord_pos_relative_to_ring using wn algo #972

Reimplement coord_pos_relative_to_ring using wn algo #972

Conversation

bjornharrtell commented Jan 23, 2023 • edited Loading

bjornharrtell commented Jan 23, 2023

bjornharrtell commented Jan 23, 2023

bjornharrtell commented Jan 23, 2023 • edited Loading

urschrei left a comment

Choose a reason for hiding this comment

urschrei Jan 23, 2023

Choose a reason for hiding this comment

bjornharrtell Jan 24, 2023

Choose a reason for hiding this comment

michaelkirk Jan 24, 2023

Choose a reason for hiding this comment

michaelkirk left a comment

Choose a reason for hiding this comment

michaelkirk Jan 23, 2023

Choose a reason for hiding this comment

michaelkirk Jan 23, 2023

Choose a reason for hiding this comment

michaelkirk commented Jan 24, 2023 • edited Loading

urschrei commented Jan 24, 2023

michaelkirk commented Jan 24, 2023

michaelkirk commented Jan 24, 2023

urschrei commented Jan 24, 2023

bors bot commented Jan 24, 2023

urschrei commented Jan 24, 2023

michaelkirk commented Jan 24, 2023

michaelkirk commented Jan 24, 2023

bjornharrtell commented Jan 23, 2023 •

edited

Loading

bjornharrtell commented Jan 23, 2023 •

edited

Loading

michaelkirk commented Jan 24, 2023 •

edited

Loading