Nc load latlon fix #4470
Merged
Nc load latlon fix #4470
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…anch 'nc_load_latlon_further_RETRO'.
bjlittle
approved these changes
Jan 4, 2022
There was a problem hiding this comment.
@pp-mo Looks good to me.
The removal of PyKE and it's re-implementation was a BIG change, so it's inevitable that these types of issues will start to surface from users reporting unexpected behaviour.
This is something that we're going to have to rely on somewhat as we're only as good as our test coverage, so thanks to @PAGWatson for letting us know 👍
@pp-mo Thanks again for the fix 🍻
|
@pp-mo Forgot to mention that this is totally worthy of a |
tkknight
added a commit
to tkknight/iris
that referenced
this pull request
Jan 5, 2022
* main: rtd with latest mambaforge image for faster building (SciTools#4476) Show acceptable image test results in the docs (SciTools#4392) Nc load latlon fix (SciTools#4470) update matplotlib links (SciTools#4474) Whatsnew for PR 4462 (SciTools#4475) Clarify the return type of iris.load (AVD-1899) (SciTools#4462) [pre-commit.ci] pre-commit autoupdate (SciTools#4472) Updated environment lockfiles (SciTools#4467)
tkknight
added a commit
to tkknight/iris
that referenced
this pull request
Jan 6, 2022
* main: Use partial to make cube pickleable (SciTools#4377) Add edit via github method to Iris docs (SciTools#4461) Broken cartopy links in docs (SciTools#4464) rtd with latest mambaforge image for faster building (SciTools#4476) Show acceptable image test results in the docs (SciTools#4392) Nc load latlon fix (SciTools#4470) update matplotlib links (SciTools#4474) Whatsnew for PR 4462 (SciTools#4475) Clarify the return type of iris.load (AVD-1899) (SciTools#4462) [pre-commit.ci] pre-commit autoupdate (SciTools#4472) Updated environment lockfiles (SciTools#4467)
pp-mo
added a commit
to pp-mo/iris
that referenced
this pull request
Jan 7, 2022
bjlittle
pushed a commit
that referenced
this pull request
Jan 7, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🚀 Pull Request
Description
Closes #4460
I realised that the reported problem was due to a mistake which I introduced in #4198 :
I had retained unchanged the trusted older routines, 'is_latitude/longitude' and 'is_rotated_latitude/longitude' , but changed the way in which they are used : As written, 'is_rotated' relies on your having already ensured the truth of 'is_latitude_longitude'.
The small changes here to
iris.fileformats._nc_load_rules.actionsfix that.Since there were no specific tests before, I have now developed a test harness for this aspect of the translation process.
Having encountered the problems described at #4469 (see below), I have only included simpler testcases, and particularly excluded some more complex ones which show potentially problematic changes in behaviour. However, I think the remaining tests do show utility, and demonstrate that the problem raised was fixed.
(
Note:
During this process I stumbled over a whole lot more unexpected complexity.
It turns out that within #4198 I may have oversimplified the logic and behaviours of the old Pyke rules, and that should now probably be reviewed : see #4469
)