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

Add generic test cases + switch to robust predicates #8

Merged
merged 21 commits into from
Jan 27, 2020

Conversation

bluenote10
Copy link
Contributor

This PR mirrors:

  • Use robust-predicates in signed area w8r/martinez#111 which has added robust predicates for the point-on-line check (signed_area). I'm not fully sure whether using robust predicates in this case is the best solution (because of this observation), but for now I would say it makes sense to stay close to the JS implementation.
  • Added generic test case handling w8r/martinez#117 which introduces a GeoJSON based test suite that replaces all the manually coded edge cases and allows for easier visualization of the test cases. This step has revealed that some of the test cases were actually a bit broken.

A test case file is a GeoJSON file that contains both the input and the expected output, as explained here. Tests only succeed if the implementation produces exactly the specified values. The output data can be regenerated by running with REGEN=true cargo test (identically to REGEN=true npm run test on JS side). The big benefit is that no code has to be written for testing edge cases, and we can get a simple overview over what we test against (*):

test_cases.pdf

Differences to the JS PR:

  • I had to disable two test cases (collapsed_edges_removed.geojson and disjoint_union_nesting.geojson) for Rust, but basically because these tests are quite broken anyway (the expected result on JS side isn't even valid GeoJSON), see the PDF attached to #117. These cases have to be revisited later anyway.
  • Two of the test cases needed a tiny numeric adjustment after copying the test case from JS. I checked the differences and they are all caused by numerical fluctuations -- I guess that has to be expected, e.g. there could be a tiny difference in operator associativity between the implementations.

Notes on the Rust implementation: I haven't found a way to make the dynamically discovered test cases show up as separate tests on cargo level (I think the available #[test]'s must be statically known). This is a bit unfortunate, because now the first assert_eq failure will break the loop over test cases. We could use a boolean any_test_failed, but then we won't get the nice diffing of pretty_assertions::assert_eq.

(*) If desired I can commit the plotting script as well. It is a tiny Python script with usage plot_test_cases.py <geojson1> <geojson2> ....

}
}

pub fn load_generic_test_case(filename: &str, regenerate: bool) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Function name isn't fully appropriate anymore => rename to run_generic_test_case or so.

Copy link
Contributor

Choose a reason for hiding this comment

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

run_generic_test_case sounds about right

@bluenote10 bluenote10 requested a review from untoldwind January 24, 2020 07:48
Copy link
Contributor

@untoldwind untoldwind left a comment

Choose a reason for hiding this comment

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

Apart from that minor refactoring looks good to me.

Would you like to fix that or should I just merge and do the refactoring myself?

F::from(-1.).unwrap()
} else {
F::from(0.).unwrap()
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I'd write this part more like this:

#[inline]
pub fn coordinate_to_robust<F>(p : Coordinate<F>) -> Coord 
where
    F: Float,
{
    Coord{x: p.x.to_f64().unwrap(), y: p.y.to_f64().unwrap()}
}

#[inline]
pub fn signed_area<F>(p0: Coordinate<F>, p1: Coordinate<F>, p2: Coordinate<F>) -> F
where
    F: Float,
{
    let res = orient2d(
        coordinate_to_robust(p0),
        coordinate_to_robust(p1),
        coordinate_to_robust(p2),
    );
    if res > 0f64 {
        F::one()
    } else if res < 0f64 {
        -F::one()
    } else {
        F::zero()
    }
}

... but that is more like a personal preference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks good to me, I have quickly pushed all the changes...

tests/src/compact_geojson.rs Outdated Show resolved Hide resolved
}
}

pub fn load_generic_test_case(filename: &str, regenerate: bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

run_generic_test_case sounds about right

@untoldwind untoldwind merged commit a097ab4 into 21re:master Jan 27, 2020
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.

2 participants