EPPT-2588: Fire Severity Index: Add a BuildUpIndex class to IMPROVER #2253
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## EPPT_2411_fire_severity_index_workflow_development #2253 +/- ##
=====================================================================================
Coverage ? 95.25%
=====================================================================================
Files ? 152
Lines ? 15369
Branches ? 0
=====================================================================================
Hits ? 14640
Misses ? 729
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mo-philrelton
changed the base branch from
master
to
EPPT_2411_fire_severity_index_workflow_development
mo-philrelton
force-pushed
the
EPPT_2588_buildupindex
branch
from
2a9f48c to
f7fc7a7
Compare
…ding tests for input validation
Anzerkhan27
left a comment
Anzerkhan27
left a comment
There was a problem hiding this comment.
The implementation is consistent with the existing fire weather plugins, the structure aligns with the FireWeatherIndexBase. The tests provide good coverage
MoseleyS
left a comment
MoseleyS
left a comment
There was a problem hiding this comment.
I've picked up on a few things to think about. The main thing is that I don't think you should include tests of base-class functions in the concrete-class tests.
| dc_data = self.input_dc.data | ||
|
|
||
| # Condition 1: If both DMC and DC are zero, set BUI = 0 | ||
| both_zero = (dmc_data == 0.0) & (dc_data == 0.0) |
There was a problem hiding this comment.
Floating point equalities are flaky. It would be better to use np.isclose(dmc_data, 0.0, atol=1e-7), or use the if statement in the Fortran: dmc_data < 1e-7
| both_zero = (dmc_data == 0.0) & (dc_data == 0.0) | ||
|
|
||
| # Condition 2: If DMC <= 0.4 * DC use equation 27a | ||
| use_27a = dmc_data <= 0.4 * dc_data |
There was a problem hiding this comment.
The Fortran has < instead of <=. I doubt this will have any noticeable impact.
| # Ensure BUI is never negative | ||
| bui = np.clip(bui, 0.0, None) |
There was a problem hiding this comment.
I think that if condition 1 is changed to match the Fortran inequality, it will mean that bui can never be negative and this line becomes superfluous.
| """ | ||
| cubes = input_cubes(dmc_val=dmc_val, dc_val=dc_val) | ||
| plugin = BuildUpIndex() | ||
| plugin.load_input_cubes(CubeList(cubes)) |
There was a problem hiding this comment.
| plugin.load_input_cubes(CubeList(cubes)) | |
| plugin.load_input_cubes(cubes) |
| def test__calculate_no_negative_values() -> None: | ||
| """Test that BUI calculation never produces negative values.""" | ||
| # Test a range of DMC and DC values | ||
| dmc_values = np.array([0.0, 5.0, 10.0, 20.0, 50.0, 100.0]) | ||
| dc_values = np.array([0.0, 10.0, 30.0, 100.0, 300.0, 500.0]) | ||
|
|
||
| for dmc in dmc_values: | ||
| for dc in dc_values: | ||
| cubes = input_cubes(dmc_val=dmc, dc_val=dc) | ||
| plugin = BuildUpIndex() | ||
| plugin.load_input_cubes(CubeList(cubes)) | ||
| bui = plugin._calculate() | ||
| assert np.all(bui >= 0.0), f"Negative BUI for DMC={dmc}, DC={dc}" |
There was a problem hiding this comment.
This test does not check that the np.clip statement correctly zeroes any negative values, because none of the input pairs result in negative values from the equations. I also cannot come up with any values that can produce negative values because negative inputs are rejected by the plugin and complex inputs are simplified by the submethods of input_cubes.
Therefore I think this test should be removed.
| plugin = BuildUpIndex() | ||
|
|
||
| with pytest.raises(ValueError, match=expected_error): | ||
| plugin.load_input_cubes(CubeList(cubes)) |
There was a problem hiding this comment.
| plugin.load_input_cubes(CubeList(cubes)) | |
| plugin.load_input_cubes(cubes) |
| plugin = BuildUpIndex() | ||
|
|
||
| with pytest.raises(ValueError, match=expected_error): | ||
| plugin.load_input_cubes(CubeList(cubes)) |
There was a problem hiding this comment.
| plugin.load_input_cubes(CubeList(cubes)) | |
| plugin.load_input_cubes(cubes) |
| # Process should complete without warnings since output stays in valid range | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("error") # Turn warnings into errors | ||
| result = plugin.process(CubeList(cubes)) |
There was a problem hiding this comment.
| result = plugin.process(CubeList(cubes)) | |
| result = plugin.process(cubes) |
| def input_cubes( | ||
| dmc_val: float = 10.0, | ||
| dc_val: float = 15.0, |
There was a problem hiding this comment.
I think you could use pytest fixtures to simplify these tests:
| def input_cubes( | |
| dmc_val: float = 10.0, | |
| dc_val: float = 15.0, | |
| @pytest.fixture | |
| def input_cubes( | |
| dmc_val: float, | |
| dc_val: float, |
| def test__calculate( | ||
| dmc_val: float, | ||
| dc_val: float, | ||
| expected_bui: float, | ||
| ) -> None: | ||
| """Test calculation of BUI from DMC and DC. | ||
|
|
||
| Verifies BUI calculation from DMC and DC values. | ||
|
|
||
| Args: | ||
| dmc_val (float): DMC value to test. | ||
| dc_val (float): DC value to test. | ||
| expected_bui (float): Expected BUI value. | ||
| """ | ||
| cubes = input_cubes(dmc_val=dmc_val, dc_val=dc_val) | ||
| plugin = BuildUpIndex() | ||
| plugin.load_input_cubes(CubeList(cubes)) |
There was a problem hiding this comment.
With the suggested pytest fixture, this would become
| def test__calculate( | |
| dmc_val: float, | |
| dc_val: float, | |
| expected_bui: float, | |
| ) -> None: | |
| """Test calculation of BUI from DMC and DC. | |
| Verifies BUI calculation from DMC and DC values. | |
| Args: | |
| dmc_val (float): DMC value to test. | |
| dc_val (float): DC value to test. | |
| expected_bui (float): Expected BUI value. | |
| """ | |
| cubes = input_cubes(dmc_val=dmc_val, dc_val=dc_val) | |
| plugin = BuildUpIndex() | |
| plugin.load_input_cubes(CubeList(cubes)) | |
| def test__calculate( | |
| dmc_val: float, | |
| dc_val: float, | |
| input_cubes, | |
| expected_bui: float, | |
| ) -> None: | |
| """Test calculation of BUI from DMC and DC. | |
| Verifies BUI calculation from DMC and DC values. | |
| Args: | |
| dmc_val (float): DMC value to test. | |
| dc_val (float): DC value to test. | |
| expected_bui (float): Expected BUI value. | |
| """ | |
| plugin = BuildUpIndex() | |
| plugin.load_input_cubes(input_cubes) |
Similar changes would be needed to all the other tests too, and while this might be more pythonic, and also better aligned with other IMPROVER tests, it isn't compulsory.
3 participants
EPPT-2588
In order to calculate the daily Fire Severity Index, we require a Buildup Index calculation. This class, and associated tests, partially reproduce the Canadian Forest Fire Weather Index from van Wagner and Pickett's 1985 FORTRAN implementation
Testing: