-
Notifications
You must be signed in to change notification settings - Fork 300
Fix PP bmdi handling #2564
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
Fix PP bmdi handling #2564
Conversation
Could you provide the rationale for this decision? |
DPeterK
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.
@pp-mo a few minor comments...
lib/iris/fileformats/pp.py
Outdated
| """ | ||
|
|
||
| # Before we can actually write to file, we need to calculate the header | ||
| # elements. First things first, make sure the data is big-endian |
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.
It is outside of your changes admittedly, but it seems that this comment (specifically about the endianness) may now be in the wrong place given the code that immediately follows it.
lib/iris/fileformats/rules.py
Outdated
| cube = iris.cube.Cube(field.core_data(), | ||
| cube_data = field.core_data() | ||
| cube_dtype = cube_data.dtype | ||
| if cube_dtype.kind in 'bui': |
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.
Right, everywhere else this is expressed as 'biu'... could you perhaps maintain that consistency?
| # Skip this test, there appears to be a long standing bug in PP saving | ||
| # for int32, which is made worse by assigning the 'default' bmdi of | ||
| # 1e30 into int arrays | ||
| @tests.skip_biggus |
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.
Nice to see these all being fixed! 💯
| field.core_data.dtype = None | ||
| core_data_array.compute = None | ||
| # It must also have a recognisable dtype. | ||
| core_data_array.dtype = np.dtype('f4') |
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.
You could set these attributes inside the mock call... but you would lose the chance to comment on each attribute, I guess.
|
@pp-mo you also have a couple of failing tests on Travis. Looks like they're both to do with resultant dtype. |
* Various un-skips. * Checked new result for PPfields repr. * Fix mock field data in TestVertical. * Fix usage of BMDI in pp load and save. * Fix test problem. * Tidy out old comments. * Use new field.realised_dtype where appropriate. * More test fixes. * Review changes, mostly cosmetic.
Unskip tests and fix problems relating to BMDI and fill value in PP save/load.
Addresses #2507.
This code is no longer allowing any usage of BMDI with integer fields.
There seems to be no official interpretation of what a missing data value might be in that case, and we don't seem to have any test data that requires it.
These changes are reflected in the results files.
The remaining test changes are almost all to do with providing mock test fields with a ".core_data()" that returns an array, or a mock array.