Skip to content

(WIP) Expand the Nimrod loader to include Table 1 #6763

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+85 −9
Draft

(WIP) Expand the Nimrod loader to include Table 1 #6763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 41 additions & 4 deletions lib/iris/fileformats/nimrod.py
View file
Open in desktop
Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a What's New entry before the end.

Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,33 @@


# data specific header (int16) elements 108-159 (Fortran bytes 411-512)
data_header_int16s = (
table_1_data_header_int16s = (
Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

"radar_number"
"radar_sites"
"additional_radar_sites"
"clutter_map_number"
"calibration_type"
"bright_band_height"
"bright_band_intensity"
"bright_band_test_param_1"
"bright_band_test_param_2"
"infill_flag"
"stop_elevation"
""
Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's important that every byte be listed here, even those bytes that are not used, otherwise the cursor in NimrodField._read_header() will malfunction. You can see how the existing Table 2 code does this:

iris/lib/iris/fileformats/nimrod.py

Lines 139 to 141 in 654e414

"data_header_int16_08",
"data_header_int16_09",
"data_header_int16_10",

"sensor_identifier"
"meteosat_identifier"
""
"software_identifier"
"software_major_version"
"software_minor_version"
"software_micro_version"
""
"period_seconds"
)


# data specific header (int16) elements 108-159 (Fortran bytes 411-512)
table_2_data_header_int16s = (
"threshold_type",
"probability_method",
"recursive_filter_iterations",
Expand Down Expand Up @@ -215,7 +241,8 @@ def _read_header(self, infile):
self._read_header_subset(infile, general_header_float32s, np.float32)
# skip unnamed floats
infile.seek(4 * (28 - len(general_header_float32s)), os.SEEK_CUR)

threshold_set = True if self.threshold_value != -32767 else False
Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
threshold_set = True if self.threshold_value != -32767 else False
threshold_set = self.threshold_value != -32767

Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kinda jarring having this line in the middle here, if you consider its surroundings. It would be more readable to move it lower, so long as it runs before the if threshold_set check.

print(threshold_set)
Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will need to come out before the end.

# data specific header (float32) elements 60-104 (bytes 175-354)
self._read_header_subset(infile, data_header_float32s, np.float32)
# skip unnamed floats
Expand All @@ -226,6 +253,14 @@ def _read_header(self, infile):
self.source = _read_chars(infile, 24)
self.title = _read_chars(infile, 24)

# determine which of Table 1 or Table 2 is being used
if threshold_set:
table = "Table_2"
data_header_int16s = table_2_data_header_int16s
else:
table = "Table_1"
Comment on lines +258 to +261
Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_header_int16s = table_1_data_header_int16s

# data specific header (int16) elements 108- (bytes 411-512)
self._read_header_subset(infile, data_header_int16s, np.int16)
# skip unnamed int16s
Expand All @@ -239,6 +274,8 @@ def _read_header(self, infile):
)
)

return table
Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're returning this, but when you call it (in NimrodField.read(), you're not assigning the result to anything, so it's not achieving anything.

Would it not be more consistent to use a instance variable: self.table = ?

If this worked, it would remove the need to pass table around as a method argument elsewhere, so long as the NimrodField itself were available.


def _read_data(self, infile):
"""Read the data array: int8, int16, int32 or float32.

Expand Down Expand Up @@ -289,7 +326,7 @@ def _read_data(self, infile):
self.data = self.data.reshape(self.num_rows, self.num_cols)


def load_cubes(filenames, callback=None):
def load_cubes(filenames, table, callback=None):
Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""Load cubes from a list of NIMROD filenames.

Parameters
Expand Down Expand Up @@ -317,7 +354,7 @@ def load_cubes(filenames, callback=None):
# End of file. Move on to the next file.
break

cube = iris.fileformats.nimrod_load_rules.run(field)
cube = iris.fileformats.nimrod_load_rules.run(field, table)

# Were we given a callback?
if callback is not None:
Expand Down
49 changes: 44 additions & 5 deletions lib/iris/fileformats/nimrod_load_rules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,6 +660,38 @@ def add_attr(item):
cube.attributes["institution"] = "Met Office"


def table_1_attributes(cube, field):
Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""Add attributes to the cube."""
Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a to-do note somewhere highlighting that the use of attributes is in the absence of expert input, which might come in future?


def add_attr(item):
"""Add an attribute to the cube."""
if hasattr(field, item):
value = getattr(field, item)
if is_missing(field, value):
return
if "radius" in item:
value = f"{value} km"
Comment on lines +672 to +673
Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike the attributes() method, none of the attributes added here include radius in their keys.

cube.attributes[item] = value

add_attr("radar_number")
add_attr("radar_sites")
add_attr("additional_radar_sites")
add_attr("clutter_map_number")
add_attr("calibration_type")
Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

add_attr("bright_band_height")
add_attr("bright_band_intensity")
add_attr("bright_band_test_param_1")
add_attr("bright_band_test_param_2")
add_attr("infill_flag")
add_attr("stop_elevation")
add_attr("sensor_identifier")
add_attr("meteosat_identifier")
add_attr("software_identifier")
add_attr("software_major_version")
add_attr("software_minor_version")
add_attr("software_micro_version")


def known_threshold_coord(field):
"""Supply known threshold coord meta-data for known use cases.

Expand Down Expand Up @@ -894,7 +926,7 @@ def time_averaging(cube, field):
cube.attributes["processing"] = averaging_attributes


def run(field, handle_metadata_errors=True):
def run(field, table, handle_metadata_errors=True):
"""Convert a NIMROD field to an Iris cube.

Parameters
Expand Down Expand Up @@ -930,10 +962,17 @@ def run(field, handle_metadata_errors=True):
# vertical
vertical_coord(cube, field)

# add other stuff, if present
soil_type_coord(cube, field)
probability_coord(cube, field, handle_metadata_errors)
ensemble_member(cube, field)
# 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)
Comment on lines +965 to +973
Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are mutually exclusive. Would be more appropriate to use a Match Statement.

Copy link
Contributor

@trexfeathers trexfeathers Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 other generic stuff, if present
time_averaging(cube, field)
attributes(cube, field)

Expand Down
Loading