-
Notifications
You must be signed in to change notification settings - Fork 300
Moveplottests #2139
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
Moveplottests #2139
Conversation
|
interestingly, given my aim to reduce version dependency, there are 4 numerical check failures, compared to a local platform (where all these pass) which tries to mimick the travis environment |
| res = regrid_area_weighted(src, dest) | ||
| qplt.pcolormesh(res) | ||
| self.check_graphic() | ||
| self.assertArrayAlmostEqual(res.data.mean(), 280.137698) |
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.
Here and elsewhere you're using assertArrayAlmostEqual for scalars.
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.
Out of interest, why have you used 'AlmostEqual', rather than 'Equal'?
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.
Out of interest, why have you used 'AlmostEqual', rather than 'Equal'?
because every time someone compares two floats for equality in a test, a fairy dies ;)
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.
So it's a precision issue? In that case, maybe a hash of data would be better to test. NumPy arrays can't be hashed, but the underyling buffer (ndarray.data) can be, if the writeable flag of the ndarray is set to False.
I'm wondering if testing the mean/standard deviation means we wouldn't catch bugs which only changed the data slightly. It's probably nothing to worry about, but just a thought.
|
👍 for removing extraneous graphics tests! Some thoughts on the strategy proposed here...
|
| '201007020900_u1096_ng_ey00_visibility0180_screen_2km')) | ||
| self.src = iris.load_cube(path)[0] | ||
| self.src.data = self.src.data.astype(np.float32) | ||
| self.src.data = self.src.data.astype(np.float64) |
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.
| plt.contourf(res.coord('grid_longitude').points, | ||
| res.coord('grid_latitude').points, | ||
| res.coord('surface_altitude').points) | ||
| self.check_graphic() |
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.
@marqh Nice. If we have assertCML coverage then there's no need for check_graphic coverage as well.
bjlittle
left a comment
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.
@marqh I agree with the pattern to purge the use of check_graphic in a unit test when it already performs an assertCML.
In all of the other cases, where check_graphic is purged and a new statistical check is now performed in it's place i.e. a tolerance on the mean and std of the data payload - I'd like to see this common pattern sprinkled throughout the code available as a new assert method within the iris.tests.IrisTest class:
- it can easily be parameterised
- also allows us to easily add more statistical checks or rigor, if we want, into the new
assertmethod, and this is applied consistently within all relevant unit tests.
I'm happy with this approach. Do other @SciTools/iris-devs have an opinion?
|
@bjlittle I understand, this seems like a useful pattern. I have implemented |
|
@marqh LGTM 👍 |
Anscombe's quartet says no. |
…nvert-unit-test * 'master' of https://github.com/SciTools/iris: Moveplottests (SciTools#2139)
remove graphic tests in favour of numerical tests for testing other parts of the code base
assertCML (where already in use) is left as sufficient test coverage
otherwise, data operations are tested using statistics of the data array