Hybrid Pressure load and save support for PP #921
Conversation
|
One test failure in |
|
This PR has had a major overhaul following discussions with an end user. I'll give it another look tomorrow with fresh eyes. |
ghost
assigned
lib/iris/aux_factory.py
Outdated
There was a problem hiding this comment.
@esc24 Although we may have discussed this offline, this change (along with its associate in the _remap_with_bounds method) takes a bit of detective work to understand the reason for the fix.
Can I float an alternative?
Simply back out changes L319-323 and L369-373 and replace the first line of _shape with:
def _shape(self, nd_values_by_key):
nd_values = sorted(nd_values_by_key.values(),
key=lambda value: value.ndim)This for me places the fix closer to the problem i.e. in the _shape method itself. This one-liner guarantees that non-None factory coordinates (with fully extensive shape) are popped first, and serve as a valid template for aggregating the derived coordinate shape.
|
Did something happen to the history, or is it GitHub? I'm seeing 21 commits. |
|
@bblay - commit early, commit often 😉 |
There was a problem hiding this comment.
@esc24 You could safely factor this function into the setUp. It's the same for all your tests i.e. the dimensions associated with each coordinate are the same.
Although you have tests that may or may not have a coordinate HybridPressureFactory.make_coord should behave correctly when one of its members has None for a coordinate. Infact, this is a bonus test of the factory functionality.
|
@esc24 Okay I'm done for now. Great work! Just a couple of minor comments to resolve. When it comes to editing the PP/GRIB rules, well we just need to raise awareness and migrate any functionality out of iris-code-generators (or not). Is this a potential Google Groups discussion for the community? The only outstanding issue for me is the |
|
@bjlittle - please take a look. I've done everything I feel needs doing. It won't merge yet due to conflicts in test_convert relating to soil levels, but I'll leave the rebase until after you're happy with what I've done here. |
|
👍 Great work @esc24 Rebase and squash the commits and then I'll merge. |
|
Ok @bjlittle - I've rebased and while resolving a conflict spotted an issue in one of the tests where I was testing for a reference pressure rather than a level pressure. I've added an extra commit so you can see the fixes I've applied. I've also squashed the earlier commits you've reviewed as far as I'm comfortable doing so. Let's see what travis thinks. |
|
@esc24 ... so close! Okay, I was erring on the side of caution and performed a local merge of your branch with upstream master, since #900 landed. The good news is that your branch merged just fine, the bad news is the |
…ort for alternative stash codes for surface_air_pressure reference
|
This is interesting, as it conflicts with the Travis report. |
|
Can you replicate this fault @bblay? Or is it just me ... |
|
All the tests pass for me, but I think I'm testing a newer version to you now. |
|
I suspect what happened was that I pushed up a version (pre biggus) that passed (both the branch and the merged version) hence the green light. However, biggus was then merged and this meant my PR was broken. I'm guessing travis doesn't rerun the tests every time something gets merged into upstream/master so the light stayed green. @bjlittle - I've made the necessary tweak to the mocked field (changing |
|
👍 Bingo! ... lovely PR @esc24, thanks! |
Hybrid Pressure load and save support for PP
|
Many thanks @bjlittle. |
5 participants
This PR adds support for loading and saving PP files with a hybrid pressure coordinate as indicated by an lbvc value of 9. This is a big PR resulting from three people working in collaboration and I'll be happy to split it up if necessary.
Remaining tasks:
Determine appropriate value ofPP_HYBRID_COORDINATE_REFERENCE_PRESSURE. I've currently set this to unity.Fix p/p* scaling on save