Triggered Lightning: Implement Layer Mean Temperature Plugins#2321
Triggered Lightning: Implement Layer Mean Temperature Plugins#2321Anzerkhan27 wants to merge 9 commits intomasterfrom
Conversation
…-lightning-layer-mean-temperature-plugin
robertplatt-mo
left a comment
There was a problem hiding this comment.
@Anzerkhan27 I've requested a few changes the main comments being in layer_mean_temperature and probably the most time consuming being type hints which I understand improver style guide expects.
Generally it all looks pretty sensible though.
| Initialise the plugin. | ||
|
|
||
| Args: | ||
| metres_to_ft (float): Conversion factor from metres to feet. |
There was a problem hiding this comment.
This feels like a curious thing to have as an argument. Surely the conversion value for metres in to ft is always the same? Why would we want to make it available to users?
There was a problem hiding this comment.
I agree with you. The conversion value will always remain the same and its no point of adding it as argument. I have moved it to the top of the file as a constant
| raise ValueError(f"bottom ({bottom} ft) must be less than top ({top} ft).") | ||
|
|
||
| if verbosity: | ||
| print(f"Extracting/interpolating levels from {bottom} to {top} ft") |
There was a problem hiding this comment.
I'm not particularly keen on this catch all tbh. I'd suggest either remove Extracting/ so this comment better reflects that the main role of the function is to interpolate the levels (given my comments elsewhere that say that extracting is just in service of the temperature interpolation), or keep extraction here and add additional verbose entries for extracting base temp and extracting top temp.
| (base, interior, and top). | ||
| """ | ||
| if bottom >= top: | ||
| raise ValueError(f"bottom ({bottom} ft) must be less than top ({top} ft).") |
There was a problem hiding this comment.
| raise ValueError(f"bottom ({bottom} ft) must be less than top ({top} ft).") | |
| raise ValueError(f"Bottom ({bottom} ft) must be less than top ({top} ft).") |
| from improver import BasePlugin | ||
|
|
||
|
|
||
| class LayerExtractionAndInterpolation(BasePlugin): |
There was a problem hiding this comment.
I'm not keen on the name of this plugin. Firstly extract and interpolate feels like it does two things. Generally it's good if functionality can be explained as doing one thing. Extraction seems to me to be something we do in service of the more important interpolation. The name also doesn't communicate that the method is primarily a tool for temperature interpolation at layer boundaries. This makes the tool seem like it's generic when it surely is not.
How about BoundaryTemperatureInterpolation?
There was a problem hiding this comment.
Or LayerTemperatureInterpolation or TemperatureInterpolationAtLayers?
There was a problem hiding this comment.
I think you are correct, the main overarching thing the function does here is Interpolate temperature at given heights. The extraction depends upon the boundary arguments that we provide. So it make sense to rename the plugin to LayerTemperatureInterpolation
| Extract and interpolate temperature values at layer boundaries. | ||
|
|
||
| Args: | ||
| temp_cube (iris.cube.Cube): Input temperature cube with a height coordinate. |
There was a problem hiding this comment.
Ryan has previously informed me that the way improver docs are generated we don't need type hints in the args as these can be dynamically generated for the types (for more info on this though probably best to ask him).
I noticed this and then subsequently noticed we haven't got any type hints. My understanding is the improver style guide has these as a requirement. Please consider this comment to apply to all docstrings and all methods (sorry I know it's a pain).
| iris.Constraint( | ||
| height=lambda point: bottom / self.metres_to_ft | ||
| < point | ||
| < top / self.metres_to_ft |
There was a problem hiding this comment.
As noted above I don't think we should be passing this in (but please correct me if I'm mistaken and there is a good reason I am not aware of). I'd set it at the top of the file as a constant.
| if verbosity: | ||
| print("Layer mean temperature array:", lmt_array) | ||
|
|
||
| # Wrap result in a cube (add metadata as needed) |
There was a problem hiding this comment.
| # Wrap result in a cube (add metadata as needed) | |
| # Wrap result in a cube and add required metadata |
As needed rather implies the user will choose what metadata is added imv
Thank you @robertplatt-mo . I have added the |
|
|
||
| Args: | ||
| layer_cube: Cube containing temperature at all heights within | ||
| the specified layer (including interpolated base and top). |
|
|
||
| # Add contributions from base and top | ||
| layer_temp_product += ( | ||
| layer_cube.data[0, :, :] * (altitude_array[1] - altitude_array[0]) / 2 |
There was a problem hiding this comment.
not sure this factor of 2 should be present (and as below) - as we are looking at adjacent layers in the calculations. In the calculation above we are looking at layers which are 2 gaps apart.
I did some checks and the calculation looks more or less right. It's rather unconventional as a double sized-gap is used for most of the calculation, buy everything is divided by 2, even the single-size gaps at the top and bottom, but the terms balance out.
| """ | ||
| height_points = np.array([100, 200, 300], dtype=np.float32) | ||
| y_points = np.array([0, 1], dtype=np.float32) | ||
| x_points = np.array([0, 1], dtype=np.float32) |
There was a problem hiding this comment.
If lines 22-24 were customised to the shape of the input data, then
make_layer_cube would be more general. I think make_layer_cube
could be used in more places in the unit tests.
| # Should include interpolated base (600ft), all interior levels, and interpolated top (800ft) | ||
| # In this synthetic example, all heights are present, so result should have 5 heights | ||
| assert result.shape == (5, 2, 2) | ||
| # Check height coordinate values |
EPPT-3150
This PR implements the mean layer temperature calculation step as part of the ongoing migration of the Helicopter-Triggered Lightning (GPP → EPP) workflow (
eppuk_triggered_lightning).The GPP implementation contained a single monolithic function
calculate_layer_mean_temperature()which handled both the vertical layer extraction and the weighted mean calculation in one function. This PR introduces two IMPROVER-style plugins to replace this, following EPP conventions.Two new plugins have been implemented:
1.
LayerExtractionAndInterpolationiris.analysis.Linear().2.
CalculateLayerMeanTemperatureLayerExtractionAndInterpolationas input.projection_y_coordinate,projection_x_coordinate) with scalar coordinatesforecast_period,forecast_reference_time, andtimepreserved from the input.standard_name="air_temperature"(CF convention) withunits="K".