Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract test fixtures into WKT files #703

Merged
merged 3 commits into from
Dec 27, 2021
Merged

Extract test fixtures into WKT files #703

merged 3 commits into from
Dec 27, 2021

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Dec 25, 2021

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Fixes #566

Some notes:

  • we say these are line strings in the WKT; that's not always the case, but it's still closer to the original code
  • I made this into a separate crate, in order to be able to access it from both benches and tests (there's no #[cfg(bench)]); it has publish = false to prevent accidental publishing
  • the extremes f64 and simplify vwp f64 benches were testing an f32 implementation
  • one quick hull test wasn't using the baseline (which was itself unused)
  • rust-analyzer used to balk on the larger include!s, it feels much better now
  • the benches seem pretty unstable on my computer

@lnicola lnicola force-pushed the extract-test-fixtures branch 5 times, most recently from 577278e to ba68d10 Compare December 25, 2021 11:07

bencher.iter(|| {
criterion::black_box(criterion::black_box(&polygon).extremes());
});
});

c.bench_function("extremes f64", |bencher| {
let points = include!("../src/algorithm/test_fixtures/norway_main.rs");
let polygon = Polygon::new(LineString::<f32>::from(points), vec![]);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong type here ^

@@ -39,8 +35,7 @@ fn criterion_benchmark(c: &mut Criterion) {
});

c.bench_function("simplify vwp f64", |bencher| {
let points = include!("../src/algorithm/test_fixtures/louisiana.rs");
let ls: LineString<f32> = points.into();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong type here, too ^

.map(|e| Coordinate { x: e.0, y: e.1 })
.collect();
let res = quick_hull(&mut v);
assert!(res.is_strictly_ccw_convex());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

poly2_hull wasn't used

@lnicola
Copy link
Member Author

lnicola commented Dec 25, 2021

r? @urschrei

Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @lnicola - LGTM!

I'll wait a couple days before merging in case someone (@urschrei) wants to weigh in.

@urschrei
Copy link
Member

And me!

@urschrei
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 27, 2021

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add WKT to devDependencies for test fixtures
3 participants