Add manual PartialEq for Interp
#367
Conversation
felixhekhorn
left a comment
felixhekhorn
left a comment
There was a problem hiding this comment.
With this fix the evolution does no longer crash with Error: interpolations do not match (I'm not sure which PDF the EKO corresponds to so I'm happy with a Error: grids are different)
|
@Radonirinaunimi: can you please write a regression test where one of the fields is a NaN? |
Good point! I just added a simple unit test. |
|
@cschwan Are you happy with this? |
| InterpMeth::Lagrange, | ||
| ); | ||
|
|
||
| assert!(interp.min.is_nan()); |
There was a problem hiding this comment.
| assert!(interp.min.is_nan()); | |
| assert!(interp.min.is_nan()); | |
| assert!(!interp.min().is_nan()); |
I haven't run it, but I think this should pass - this would make the "weird" behaviour a bit more plain
There was a problem hiding this comment.
You probably refer to NaN != NaN being a true statement, but NaN.is_nan() should always be true.
There was a problem hiding this comment.
nope - I want to highlight the difference between the method min and the attribute min (which was confusing to me)
There was a problem hiding this comment.
Well, to me too 😄. In that case a clarifying comment is in order.
There was a problem hiding this comment.
That's why I needed the rmin method in the issue. From the existing test we then know that approx_eq!(f64,NaN,NaN,ulps=1) is true, which may be (for me: is) surprising. From the new test we would also know that NaN.min(1.0) is 1.0, which may be (for me: is) surprising.
Do you mean something like this
| assert!(interp.min.is_nan()); | |
| // Starting from below the valid domain of the map results in a NaN for the attribute `min`, ... | |
| assert!(interp.min.is_nan()); | |
| // but recall that the corresponding method `min()` is minimising between `min` and `max`, | |
| // as the map might be inverse proportional: | |
| assert!(!interp.min().is_nan()); |
Addresses #366.