(WIP) Expand the Nimrod loader to include Table 1 #6763
Conversation
| infile.seek(4 * (28 - len(general_header_float32s)), os.SEEK_CUR) | ||
|
|
||
| threshold_set = True if self.threshold_value != -32767 else False | ||
| print(threshold_set) |
There was a problem hiding this comment.
Will need to come out before the end.
There was a problem hiding this comment.
Needs a What's New entry before the end.
| # skip unnamed floats | ||
| infile.seek(4 * (28 - len(general_header_float32s)), os.SEEK_CUR) | ||
|
|
||
| threshold_set = True if self.threshold_value != -32767 else False |
There was a problem hiding this comment.
| threshold_set = True if self.threshold_value != -32767 else False | |
| threshold_set = self.threshold_value != -32767 |
| # add Table 1 specific stuff | ||
| if table == "Table_1": | ||
| table_1_attributes(cube, field) | ||
|
|
||
| # add Table 2 specific stuff | ||
| if table == "Table_2": | ||
| soil_type_coord(cube, field) | ||
| probability_coord(cube, field, handle_metadata_errors) | ||
| ensemble_member(cube, field) |
There was a problem hiding this comment.
These are mutually exclusive. Would be more appropriate to use a Match Statement.
|
|
||
|
|
||
| def load_cubes(filenames, callback=None): | ||
| def load_cubes(filenames, table, callback=None): |
There was a problem hiding this comment.
I believe load_cubes is the common signature that all Iris loaders must provide. So I would probably expect this addition to break NIMROD loading, if not all Iris loading, depending how the io module does its thing.
| cube.attributes["institution"] = "Met Office" | ||
|
|
||
|
|
||
| def table_1_attributes(cube, field): |
There was a problem hiding this comment.
I think some changes will be needed to the existing attributes function. I notice that at least one of the attributes covered there - probability_period_of_event - is specific to Table 2.
I assume this doesn't cause problems at run time - if the attribute is absent then nothing happens - but it does make it misleading for future developers.
| if table == "Table_2": | ||
| soil_type_coord(cube, field) | ||
| probability_coord(cube, field, handle_metadata_errors) | ||
| ensemble_member(cube, field) |
There was a problem hiding this comment.
ensemble_member is part of general_header_int16s; how come we have it as specific to Table 2?
(I have checked the rest of the functions called within run - pretty sure it's just ensemble_member() and attributes() that might be a problem).
| add_attr("radar_sites") | ||
| add_attr("additional_radar_sites") | ||
| add_attr("clutter_map_number") | ||
| add_attr("calibration_type") |
There was a problem hiding this comment.
We have further information on how to parse the calibration type:
Calibration Type (0=uncalibrated, 1=frontal, 2=showers, 3=rain shadow, 4=bright band ; the negatives of these values can be used to indicate a calibration which has subsequently been removed.
I would have assumed it should stay as the raw numbers, but that's definitely inconsistent with the existing patterns in this file.
| table = "Table_2" | ||
| data_header_int16s = table_2_data_header_int16s | ||
| else: | ||
| table = "Table_1" |
There was a problem hiding this comment.
If you ever find yourself hardcoding strings like this - i.e. downstream code is checking for the exact same string - it is safer to use an Enum of some type. You can use enum.auto() for generating the values.
|
|
||
| # data specific header (int16) elements 108-159 (Fortran bytes 411-512) | ||
| data_header_int16s = ( | ||
| table_1_data_header_int16s = ( |
There was a problem hiding this comment.
Each value needs to be comma-separated, otherwise this gets interpreted as a string and the code definitely doesn't run as intended. Have you tried running this locally?
2 participants
🚀 Pull Request
Description
This adds onto the existing code, and seperates things between Table 1 and Table 2 that was always treated as default before.
Consult Iris pull request check list
Add any of the below labels to trigger actions on this PR: