-
Notifications
You must be signed in to change notification settings - Fork 199
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
Incorrect usage of abs_sub
#120
Conversation
`x.abs_sub(y)` is defined ([confusingly](rust-lang/rust#30315)) as `max(x-y, 0)`, not `abs(x-y)`. This wasn't caught in tests since `t_x - t_y` in [src/algorithm/intersects.rs:48](https://github.com/georust/rust-geo/blob/master/src/algorithm/intersects.rs#L48) is positive in all of the given examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
src/algorithm/intersects.rs
Outdated
@@ -442,6 +442,7 @@ mod test { | |||
#[test] | |||
fn point_intersects_line_test() { | |||
let p0 = Point::new(2., 4.); | |||
let p1 = Point::new(0., 0.); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No big deal, but rather than adding another point, it seems like the existing pattern of this test is to have one point, and then a line for every test case with a comment explaining why that particular line is interesting.
As written, it's not clear why that point is an interesting test case outside of the context of this PR.
Maybe this would be a little clearer:
// Line below point
let line4 = Line::new(Point::new(0.,0.), Point::new(5., 0.)
assert!(!line4.intersects(&p0));
// Line above point
let line5 = Line::new(Point::new(0.,5.), Point::new(5., 5.)
assert!(!line5.intersects(&p0));
assert!(!line6.intersects(&p0)); | ||
assert!(!p0.intersects(&line6)); | ||
assert!(!line7.intersects(&p0)); | ||
assert!(!p0.intersects(&line7)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelkirk: to follow up on your previous comment, the failure occurs when the point is above a line with positive slope or below a line with negative slope. So I included examples above and below lines with positive and negative slopes. On master, lines 465-466 (line4
) and lines 471-472 (line7
) will fail.
Wow, good catch! Thanks! 🙇♂️ |
bors: r+ |
bors r+ |
120: Incorrect usage of `abs_sub` r=frewsxcv `x.abs_sub(y)` is defined ([confusingly](rust-lang/rust#30315)) as `max(x-y, 0)`, not `abs(x-y)`. This wasn't caught in tests since `t_x - t_y` in [src/algorithm/intersects.rs:48](https://github.com/georust/rust-geo/blob/master/src/algorithm/intersects.rs#L48) happens to be positive in all of the given examples so I've added a new example where `t_x - t_y` is negative.
Build succeeded |
x.abs_sub(y)
is defined (confusingly) asmax(x-y, 0)
, notabs(x-y)
. This wasn't caught in tests sincet_x - t_y
in src/algorithm/intersects.rs:48 happens to be positive in all of the given examples so I've added a new example wheret_x - t_y
is negative.