Replaced spade with rstar #314

Stoeoef · 2018-11-22T23:24:14Z

Hi there!

I'm in the process of extracting spade's rtree and was finally able to get the geo tests running with the planned replacement crate: rstar.
Since the results so far look really promising, I'd like to share them with this PR to get some first reactions.
The goodies:

Reduced compilation times for geo workspace: cargo clean && cargo build was reduced to 11s (from 40s) on my local machine
Benchmark improvements:

simplify vwp f32 time: [17.300 ms 17.413 ms 17.551 ms]
change: [-70.837% -70.698% -70.558%] (p = 0.00 < 0.05)
Performance has improved.

The smaller build times may actually justify to use rstar without a feature flag.

Some TODO's before I'm willing to remove the WIP status:

I'll perform a review of the changes myself again
The simplify vwp algorithm was adapted a bit, I'll need to investigate if it is still correct. I'll add a comment to the appropriate section and ask for a special review.
I need to check the test coverage of rstar for the methods used by geo, Should not be too dramatic, but I want those tests anyway...

Also, rstars public interface is likely to change in the near future - however, I would keep geo updated should any breaking change make this necessary. That is, after all, in my own interest, as failing geo test cases will likely mean that something is wrong with my changes.

Any thoughts or opinions are welcome. Please keep in mind that, as I just published the rstar crate, it will take a while until it is in a polished state. I hope does not pose too much of a problem.

urschrei · 2018-11-23T11:54:33Z

Oh, amazing. Thank you! Feel free to ping me about the vwp changes – if the tests are still passing, I'm pretty confident that it'll be OK, but I can certainly cook up some more to exercise it even more thoroughly.

frewsxcv · 2018-11-23T13:37:01Z

Wow, this looks awesome! Extracting out the r*-tree was a great idea 👍

I just skimmed through the code and everything is look good, ping me or @urschrei if you need any help! 👋

Stoeoef

I've removed the WIP status as I am happy with the changes. Feel free to modify the Cargo.toml versions, changing them to something final will require a cargo publish I believe. Also, if there are any other contribution rules (like running cargo fmt), now would be a good time to mention.

Stoeoef · 2018-11-29T21:00:56Z

geo/Cargo.toml

 failure = "0.1.2"
 postgis = { version = "0.6", optional = true }
 proj = { version = "0.5", optional = true }
-geo-types = { version = "0.2", features = ["spade"] }
+geo-types = { path = "../geo-types", features = ["rstar"] }


This will need to be updated in order to publish the crate... feel free to change it to any version you see fit.

yep, this makes sense 👍

Stoeoef · 2018-11-29T21:30:58Z

geo/src/algorithm/simplifyvw.rs

+        let line_1 = Line::new(left_point, middle_point);
+        let line_2 = Line::new(middle_point, right_point);
+        assert!(tree.remove(&line_1).is_some());
+        assert!(tree.remove(&line_2).is_some());


Note that I originally had a different assertion:
assert!(tree.remove(&line_1).is_some() || tree.remove(&line1.reverse().is_some())
as I was not sure in which direction a line would have been loaded.

I then realized that, as long as all lines are going "in the same direction", these assumptions should hold: If edges A->B and B->C exist, then they should be loaded as (A,B), (B,C) in contrast to (A,B),(C,B)).

Since all incoming lines are either the lines of a line string (which is directed) or the inner rings of a polygon (which are also directed), these assertions should hold. However, you may want to check this yourselves. Please note that the spade algorithm was not susceptible to this, that's why I'm so cautious. It was though, at the same time, much more inefficient.

As you say, this could only be a problem for us if someone constructs an invalid geometry. @frewsxcv, I know we aren't currently checking for that, but I don't know if this is the right place to enforce it – I think it's reasonable for individual algorithms to assume that incoming geometries are valid.

agreed. we still need to work on our validation story in this crate, but for the time being, i think it's okay to assume geometry coming in is invalid so this looks good 👍

frewsxcv

amazing work @shterrett, thanks for making this improvement and contribution! ✨

as this is a breaking change for both 'geo' and 'geo-types', i'm not yet going to publish a new version – i'm thinking about including other breaking changes in the next release and would prefer to group them up in one release

frewsxcv · 2018-12-02T18:47:21Z

geo/src/algorithm/simplifyvw.rs

+        let line_1 = Line::new(left_point, middle_point);
+        let line_2 = Line::new(middle_point, right_point);
+        assert!(tree.remove(&line_1).is_some());
+        assert!(tree.remove(&line_2).is_some());


agreed. we still need to work on our validation story in this crate, but for the time being, i think it's okay to assume geometry coming in is invalid so this looks good 👍

frewsxcv · 2018-12-02T18:49:23Z

bors r+

314: Replaced spade with rstar r=frewsxcv a=Stoeoef Hi there! I'm in the process of extracting spade's rtree and was finally able to get the geo tests running with the planned replacement crate: `rstar`. Since the results so far look really promising, I'd like to share them with this PR to get some first reactions. The goodies: - Reduced compilation times for `geo` workspace: `cargo clean && cargo build` was reduced to 11s (from 40s) on my local machine - Benchmark improvements: > simplify vwp f32 time: [17.300 ms 17.413 ms 17.551 ms] > change: [-70.837% **-70.698%** -70.558%] (p = 0.00 < 0.05) > Performance has improved. The smaller build times may actually justify to use `rstar` without a feature flag. Some TODO's before I'm willing to remove the WIP status: - [x] I'll perform a review of the changes myself again - [x] The simplify vwp algorithm was adapted a bit, I'll need to investigate if it is still correct. I'll add a comment to the appropriate section and ask for a special review. - [x] I need to check the test coverage of `rstar` for the methods used by `geo`, Should not be too dramatic, but I want those tests anyway... Also, `rstars` public interface is likely to change in the near future - however, I would keep `geo` updated should any breaking change make this necessary. That is, after all, in my own interest, as failing geo test cases will likely mean that something is wrong with my changes. Any thoughts or opinions are welcome. Please keep in mind that, as I just published the `rstar` crate, it will take a while until it is in a polished state. I hope does not pose too much of a problem. Co-authored-by: Stefan Altmayer <stoeoef@gmail.com>

bors · 2018-12-02T19:04:31Z

Build succeeded

continuous-integration/travis-ci/push

frewsxcv mentioned this pull request Nov 23, 2018

Update spade to 1.6 #308

Closed

Stoeoef force-pushed the master branch from 1304b8a to 5d4f48a Compare November 29, 2018 20:50

Stoeoef changed the title ~~WIP: Replaced spade with rstar~~ Replaced spade with rstar Nov 29, 2018

Replaced spade with rstar

7d1ddc6

Stoeoef force-pushed the master branch from 5d4f48a to 7d1ddc6 Compare November 29, 2018 20:53

Stoeoef commented Nov 29, 2018

View reviewed changes

frewsxcv approved these changes Dec 2, 2018

View reviewed changes

bors bot merged commit 7d1ddc6 into georust:master Dec 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaced spade with rstar #314

Replaced spade with rstar #314

Stoeoef commented Nov 22, 2018 •

edited

urschrei commented Nov 23, 2018

frewsxcv commented Nov 23, 2018

Stoeoef left a comment

Stoeoef Nov 29, 2018

frewsxcv Dec 2, 2018

Stoeoef Nov 29, 2018

urschrei Nov 30, 2018

frewsxcv Dec 2, 2018

frewsxcv left a comment

frewsxcv Dec 2, 2018

frewsxcv commented Dec 2, 2018

bors bot commented Dec 2, 2018

Replaced spade with rstar #314

Replaced spade with rstar #314

Conversation

Stoeoef commented Nov 22, 2018 • edited

urschrei commented Nov 23, 2018

frewsxcv commented Nov 23, 2018

Stoeoef left a comment

Choose a reason for hiding this comment

Stoeoef Nov 29, 2018

Choose a reason for hiding this comment

frewsxcv Dec 2, 2018

Choose a reason for hiding this comment

Stoeoef Nov 29, 2018

Choose a reason for hiding this comment

urschrei Nov 30, 2018

Choose a reason for hiding this comment

frewsxcv Dec 2, 2018

Choose a reason for hiding this comment

frewsxcv left a comment

Choose a reason for hiding this comment

frewsxcv Dec 2, 2018

Choose a reason for hiding this comment

frewsxcv commented Dec 2, 2018

bors bot commented Dec 2, 2018

Build succeeded

Stoeoef commented Nov 22, 2018 •

edited