Skip to content

EPPT-2587: Fire Severity Index: Add a InitialSpreadIndex class to IMPROVER #2252

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

Jump to bottom
Open
mo-philrelton wants to merge 8 commits into
base: EPPT_2411_fire_severity_index_workflow_development
Choose a base branch
from
Open

EPPT-2587: Fire Severity Index: Add a InitialSpreadIndex class to IMPROVER #2252

mo-philrelton wants to merge 8 commits into from

Conversation

@mo-philrelton
Copy link
Contributor

@mo-philrelton mo-philrelton commented Nov 28, 2025 •
edited
Loading

EPPT-2587

In order to calculate the daily Fire Severity Index, we require an Initial Spread 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:

  • Ran tests and they passed OK
  • Added new tests for the new feature(s)

@codecov
Copy link

codecov bot commented Nov 28, 2025 •
edited
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (EPPT_2411_fire_severity_index_workflow_development@a2fcead). Learn more about missing BASE report.

Additional details and impacted files 
@@                                  Coverage Diff                                  @@
##             EPPT_2411_fire_severity_index_workflow_development    #2252   +/-   ##
=====================================================================================
  Coverage                                                      ?   95.25%           
=====================================================================================
  Files                                                         ?      152           
  Lines                                                         ?    15376           
  Branches                                                      ?        0           
=====================================================================================
  Hits                                                          ?    14647           
  Misses                                                        ?      729           
  Partials                                                      ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mo-philrelton mo-philrelton changed the base branch from master to EPPT_2411_fire_severity_index_workflow_development December 1, 2025 10:22
@mo-philrelton mo-philrelton force-pushed the EPPT_2587_initialspreadindex branch from 9132f02 to 22f9a43 Compare December 16, 2025 16:59
RDP-mo
RDP-mo reviewed Dec 19, 2025
View reviewed changes
Copy link

@RDP-mo RDP-mo left a comment

Choose a reason for hiding this comment

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

Looks good - mostly nits again. I'd only add that I'd avoid duplicating in comment form what is already in docstring form and generally I'd go for a few less comments. Comment drift is a real and common problem. Wrong comments are more problematic in my experience than missing comments (at least if the code is reasonably clear too). Also reading 2x the text is double the complexity rather than half so sometimes additional comments aren't helping the reader as much as intended.

@RDP-mo RDP-mo mentioned this pull request Dec 19, 2025
Open
2 tasks
ryan-cocking-mo
ryan-cocking-mo approved these changes Dec 30, 2025
View reviewed changes
Copy link
Contributor

@ryan-cocking-mo ryan-cocking-mo left a comment

Choose a reason for hiding this comment

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

Tests all pass, just a few comments around types in docstrings. Approved.

improver/fire_weather/initial_spread_index.py
Comment on lines +103 to +108
Args:
spread_factor (np.ndarray): The spread factor values.
wind_function (np.ndarray): The wind function values.

Returns:
np.ndarray: The calculated ISI values.
Copy link
Contributor

@ryan-cocking-mo ryan-cocking-mo Dec 30, 2025

Choose a reason for hiding this comment

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

Suggested change
Args:
spread_factor (np.ndarray): The spread factor values.
wind_function (np.ndarray): The wind function values.
Returns:
np.ndarray: The calculated ISI values.
Args:
spread_factor: The spread factor values.
wind_function: The wind function values.
Returns:
The calculated ISI values.

Types not required in docstring; Sphinx/Read the Docs infers from type hints.

improver/fire_weather/initial_spread_index.py
From Van Wagner and Pickett (1985), Page 7: Equation 25.

Returns:
np.ndarray: The spread factor values.
Copy link
Contributor

@ryan-cocking-mo ryan-cocking-mo Dec 30, 2025

Choose a reason for hiding this comment

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

Suggested change
np.ndarray: The spread factor values.
The spread factor values.

Type not required in docstring.

improver/fire_weather/initial_spread_index.py
From Van Wagner and Pickett (1985), Page 7: Equation 24.

Returns:
np.ndarray: The wind function values.
Copy link
Contributor

@ryan-cocking-mo ryan-cocking-mo Dec 30, 2025

Choose a reason for hiding this comment

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

Suggested change
np.ndarray: The wind function values.
The wind function values.

Type not required in docstring.

improver/fire_weather/initial_spread_index.py
This uses Steps 1 & 2 from Van Wagner and Pickett (1985), page 8.

Returns:
np.ndarray: The calculated ISI values.
Copy link
Contributor

@ryan-cocking-mo ryan-cocking-mo Dec 30, 2025

Choose a reason for hiding this comment

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

Suggested change
np.ndarray: The calculated ISI values.
The calculated ISI values.

Type not required in docstring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RDP-mo RDP-mo RDP-mo left review comments

@ryan-cocking-mo ryan-cocking-mo ryan-cocking-mo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mo-philrelton @ryan-cocking-mo @RDP-mo