-
Notifications
You must be signed in to change notification settings - Fork 100
EPPT-2585: Fire Severity Index: Add a DuffMoistureCode class to IMPROVER #2250
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.
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
base: EPPT_2411_fire_severity_index_workflow_development
Are you sure you want to change the base?
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 #2250 +/- ##
=====================================================================================
Coverage ? 95.26%
=====================================================================================
Files ? 152
Lines ? 15391
Branches ? 0
=====================================================================================
Hits ? 14662
Misses ? 729
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b945990 to
553c6bd
Compare
553c6bd to
0150a08
Compare
ryan-cocking-mo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests all pass, code looks good, and equations match those in the literature. I have made some tiny suggestions around types in doc strings that I think makes them slightly neater (and works better when rendering the IMPROVER docs with Sphinx/Read the Docs).
| From Van Wagner and Pickett (1985), Page 6: Equation 16. | ||
|
|
||
| Args: | ||
| drying_rate (np.ndarray): The drying rate (RK). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| drying_rate (np.ndarray): The drying rate (RK). | |
| drying_rate: The drying rate, represented by 'K' in the original equations. |
The symbol for drying rate is K in the equations, but having (K) in the docstring may be confused for units Kelvin.
| """Calculate the Duff Moisture Code (DMC). | ||
|
|
||
| Returns: | ||
| np.ndarray: The calculated DMC values for the current day. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| np.ndarray: The calculated DMC values for the current day. | |
| The calculated DMC values for the current day. |
The IMPROVER setup for Sphinx/Read the Docs automatically infers the return type from the type hint. Removing it from the docstring will also add a hyperlink to the np.ndarray documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect we need to consider this comment on the other FireSeverity PRs if we want the docs to render correctly
| From Van Wagner and Pickett (1985), Page 6: Equation 16, Steps 3 & 4. | ||
|
|
||
| Returns: | ||
| np.ndarray: The drying rate (dimensionless). Shape matches input cube |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| np.ndarray: The drying rate (dimensionless). Shape matches input cube | |
| The drying rate (dimensionless). Shape matches input cube |
See previous comment on inferring return type from type hint.
| drying_rate (np.ndarray): The drying rate (RK). | ||
|
|
||
| Returns: | ||
| np.ndarray: The calculated DMC value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| np.ndarray: The calculated DMC value. | |
| The calculated DMC value. |
| different previous DMC values. | ||
|
|
||
| Args: | ||
| precip_val (float): Precipitation value for all grid points. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a suggestion, just a comment: the types can be kept in brackets in test docstrings, because they are not processed by Sphinx/Read the Docs in the IMPROVER repository.
| """Calculate the Duff Moisture Code (DMC). | ||
|
|
||
| Returns: | ||
| np.ndarray: The calculated DMC values for the current day. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect we need to consider this comment on the other FireSeverity PRs if we want the docs to render correctly
| assert DuffMoistureCode.DMC_DAY_LENGTH_FACTORS == expected_factors | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite tricky to read. 1 option would be
DMC_NEGATIVE = 20.0, 1.0, 50.0, -5.0
TEMPERATURE_TOO_HIGH =150.0, 1.0, 50.0, 6.0
TEMPERATURE_TOO_LOW = -150.0, 1.0, 50.0, 6.0
RELATIVE_HUMIDITY_TOO_HIGH = 20.0, 1.0, 150.0, 6.0 # Relative humidity above 100%
NEGATIVE_RELATIVE_HUMIDITY = 20.0, 1.0, -10.0, 6.0
PRECIPITATION_NEGATIVE = 20.0, -5.0, 50.0, 6.0
@pytest.mark.parametrize(
"temp_val, precip_val, rh_val, dmc_val, expected_error",
[
(*TEMPERATURE_TOO_HIGH, "temperature contains values above valid maximum"),
(*TEMPERATURE_TOO_LOW, "temperature contains values below valid minimum"),
(*PRECIPITATION_NEGATIVE, "precipitation contains values below valid minimum"),
(*RELATIVE_HUMIDITY_TOO_HIGH, "relative_humidity contains values above valid maximum"),
(*NEGATIVE_RELATIVE_HUMIDITY, "relative_humidity contains values below valid minimum"),
(*DMC_NEGATIVE, "input_dmc contains values below valid minimum"),
],
)
def test_func
Note the * to expand the tuples as args
| elif invalid_input_type == "input_dmc_nan": | ||
| dmc_val = np.nan | ||
| elif invalid_input_type == "input_dmc_inf": | ||
| dmc_val = np.inf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As covered in the other reviews these would be better as part of the parametrization of the test.
| plugin = DuffMoistureCode() | ||
|
|
||
| # Process should complete without warnings since output stays in valid range | ||
| import warnings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move import to top of file
EPPT-2585
In order to calculate the daily Fire Severity Index, we require a Duff Moisture Code calculation. This class, and associated tests, partially reproduce the Canadian Forest Fire Weather Index from van Wagner and Pickett's 1985 FORTRAN implementation
Testing: