search

cartographer-project / point_cloud_viewer

View billions of points in your browser.
Apache License 2.0
339 stars 98 forks source link

Integration test for end-to-end comparison of octrees and S2 point clouds #344

Closed nnmm closed 5 years ago

nnmm commented 5 years ago

Right now, this only includes creation of two point clouds from the same points. It will be extended to cover querying as well.

nnmm commented 5 years ago

meow @catevita @feuerste

catevita commented 5 years ago

should this binary land in some sort of /benches directory for benchmarking?

catevita commented 5 years ago

maybe we could then move there all the other kind of benchmarks

nnmm commented 5 years ago

I'm a bit unsure whether we should declare it as a benchmark ... wouldn't that just benchmark the execution time of the whole binary, instead of octree/s2 building separately?

feuerste commented 5 years ago

I'm a bit unsure whether we should declare it as a benchmark ... wouldn't that just benchmark the execution time of the whole binary, instead of octree/s2 building separately?

What's the purpose of this binary at the end? Comparing whether querying gives the exact same results? Then I would opt for moving it into a tests dir for integration tests. If we additionally want to measure time, I'm not sure, whether tests is the right place...

nnmm commented 5 years ago

Yes, that's the main purpose. Does tests get special treatment by cargo?

feuerste commented 5 years ago

See https://doc.rust-lang.org/cargo/reference/manifest.html#integration-tests for a bit of explanation.

nnmm commented 5 years ago

Okay, sounds like it can not have command line arguments then, but I think it makes sense to make it an integration test.

googlebot commented 5 years ago

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

nnmm commented 5 years ago

Changed it, ptal. However, now there's no way to modify the flags anymore. cargo test generate_and_query -- --help prints some unrelated help text four times for me.

feuerste commented 5 years ago

Changed it, ptal. However, now there's no way to modify the flags anymore. cargo test generate_and_query -- --help prints some unrelated help text four times for me.

Ok, shall we then just remove all options and make them consts?

googlebot commented 5 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

nnmm commented 5 years ago

Yep, we can do that for now. On thing I don't know how to handle within an integration test though is benchmarking. I'd like to benchmark multiple parts later: Both octree and S2 generation, and different kinds of queries. How can we avoid repeating the setup (creating the random pointclouds) for the latter?

feuerste commented 5 years ago

https://doc.rust-lang.org/unstable-book/language-features/custom-test-frameworks.html unfortunately doesn't seem to be available yet, but how about having the common setup in a lazy_static?

nnmm commented 5 years ago

Benchmark seem to not work at all inside tests/, only in src/ and benches/, so I moved it to benches. I'm using TempDir for cleanup, and lazy_static to avoid regenerating things for tests (for the benchmark, regenerating it is of course intentional).