Adding functionality to choose closest matching threshold.#2405
Adding functionality to choose closest matching threshold.#2405mo-jbeaver wants to merge 3 commits into
Conversation
brhooper
left a comment
brhooper
left a comment
There was a problem hiding this comment.
Thanks @mo-jbeaver I think that this PR makes a useful improvement to the robustness of the decision tree application plugin. I've left you a few minor comments.
| diff_index = diff.index(min(diff)) | ||
| closest_point = threshold_points[diff_index] |
There was a problem hiding this comment.
| diff_index = diff.index(min(diff)) | |
| closest_point = threshold_points[diff_index] | |
| # find closest available threshold to desired threshold | |
| closest_point = threshold_points[diff.index(min(diff))] |
diff_index is only used once, so I don't think we need to create a variable to store it.
| else: | ||
| missing_data.append( | ||
| f"name: {diagnostic}, threshold: {threshold}, " | ||
| f"(closest match extraction failed)\n" | ||
| ) |
There was a problem hiding this comment.
Is it actually possible to enter this else statement? If it is possible, I don't think that we currently have a unit test to cover it.
There was a problem hiding this comment.
I've re-checked this and I agree, so I have removed this section of the code.
| ) | ||
| num_thresholds = len(threshold_points) | ||
|
|
||
| if num_thresholds > 1: |
There was a problem hiding this comment.
I think an inline code comment here to describe how we handle finding multiple thresholds could be useful here.
There was a problem hiding this comment.
Inline comment added
|
|
||
| threshold_coord = find_threshold_coordinate(self.cubes[0]) | ||
| additional_threshold = threshold_coord.points[0] * ( | ||
| original_threshold_coord = threshold_coord.points[0] |
There was a problem hiding this comment.
I think original_threshold is a better name for this variable. I don't think that it stores a coordinate, just a single point from a coordinate.
There was a problem hiding this comment.
Variable name updated
| for cube in result | ||
| if threshold_coord.name() in [coord.name() for coord in cube.coords()] | ||
| ] | ||
| self.assertIn(original_threshold_coord, extracted_thresholds) |
There was a problem hiding this comment.
You could also assert that extracted_thresholds has length one.
There was a problem hiding this comment.
Unit test adjusted to better fit the outputs, and a length check added.
brhooper
left a comment
brhooper
left a comment
There was a problem hiding this comment.
Thanks @mo-jbeaver I've added another couple of minor comments.
| expected_thresholds = {original_threshold, threshold_coord.points[2]} | ||
| modified_cube = [cube for cube in result if "lwe_snowfall_rate" in cube.name()] | ||
| for cube in modified_cube: | ||
| self.assertGreater(len(modified_cube), 0) |
There was a problem hiding this comment.
This should be outside the for loop. I also think that we expect exactly one cube in modified_cube.
There was a problem hiding this comment.
I have moved the assertion to outside of the for loop.
For the modified_cube [now called snowfall_cubes], it pulls all snowfall related cubes from result. There are multiple cubes, as each cube refers to a node on the decision tree.
| cube_thresholds = cube.coord(find_threshold_coordinate(cube).name()).points | ||
| self.assertEqual(len(cube_thresholds), 1) | ||
| cube_threshold_value = cube_thresholds[0] | ||
| self.assertIn(cube_threshold_value, expected_thresholds) |
There was a problem hiding this comment.
I think we should be able to know which threshold we expect to be in the output cube, and to check that the actual returned threshold is the expected one.
There was a problem hiding this comment.
As above, there are multiple cubes in snowfall_cubes each its own threshold value, depending on its position on the decision tree. Here, we are asserting that we expect that this threshold value is either the original_threshold or threshold_coord.points[2], as the plugin will have selected the original_threshold over the additional_threshold.
2 participants
Addresses #1176
Adding changes to the plugin to choose the closest threshold, when there are multiple matching thresholds available .
Testing: