-
Notifications
You must be signed in to change notification settings - Fork 284
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
Support cartopy 0.20 and proj 8.1.0 #4331
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,16 +68,23 @@ def test_nearest_gnomonic_uk_domain(self): | |
res = self.src.regrid(uk_grid, ProjectedUnstructuredNearest(crs)) | ||
|
||
self.assertArrayShapeStats( | ||
res, (1, 6, 17, 11), 315.8873266, 11.0006664668, rtol=1e-8 | ||
res, | ||
(1, 6, 17, 11), | ||
315.8854720963427, | ||
11.000539210625737, | ||
rtol=1e-8, | ||
) | ||
self.assertArrayShapeStats( | ||
res[:, 0], (1, 17, 11), 299.99993826, 4.1356150388e-5 | ||
res[:, 0], | ||
(1, 17, 11), | ||
299.9999985207442, | ||
3.53574517015874e-05, | ||
) | ||
expected_subset = np.array( | ||
[ | ||
[318.936829, 318.936829, 318.936829], | ||
[318.936829, 318.936829, 318.936829], | ||
[318.935163, 318.935163, 318.935163], | ||
[318.92881733, 318.92881733, 318.92881733], | ||
[318.92881733, 318.92881733, 318.92881733], | ||
[318.92881733, 318.92881733, 318.92881733], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't identify the root cause for the change in numbers here... hence why I've introduced a minimum pin for We either accept this change "as is"... or dig deeper. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the provenance of the original numbers in the test? Assuming these are degrees longitude the difference is 0.00635deg ~ 700m on the equator. Do we at least understand why the original test array was 2 rows of 318.936829 and 1 row of 318.935163, while the replacement is 3 identical rows of 318.92881733? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think there is any significance to the values being tested in the array, other than a random snapshot. @pp-mo Any ideas on the history of this one? I think the philosophy of the test is rather weak, in that it's attempting to give "some level" of confidence that nothing has changed... but when something does change (like now) there is no context to determine whether the change is significant and/or valid. I'm more inclined to trust a plot rather than raw numbers for this type of integration test 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
👍 😁
I don't think there is anything special here, just a desire to check a small section of individual points, not just statistics over the whole array. An actual change equivalent to 100's of metres does seem rather large though. Maybe the default ellipsoid for this changed ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then again... the plots are rather different too... The six subplots within each plot are the six Apologies, forgot to add a colorbar, oops. There is some similar structure there, but the differences are (to me) significant. At this point, I can't confirm what's right. Any clues @pp-mo ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
how did this manage to match on the array stats, I wonder ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've rebased, as this PR went stale... I'm still in favour of creating an issue to cover this, if that seems like a pragmatic compromise. Then we can dedicate some time to digging a bit further to understand the root cause of the issue (if possible), but bank this PR and move on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If we're honest, that's effectively saying we agree this change isn't a problem, and maybe we'll get round to working out what caused it at some point in the future. I'm fine with that if everyone else is. However: if this is agreed to be a real concern then I'm very uncomfortable merging into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the fact that this is in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rcomer Nice point, it's experimental... I'm not too precious about it. Also, the fact that this issue is isolated in one test and not an issue peppered throughout Iris is somewhat comforting. |
||
] | ||
) | ||
self.assertArrayAlmostEqual( | ||
|
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.
Thanks to @jamesp for this magic fix 🎉