Skip to content

Accept m/min spread rate in fireline_intensity by default (#3527)#3529

Merged
brendancol merged 4 commits into
mainfrom
issue-3527
Jun 26, 2026
Merged

Accept m/min spread rate in fireline_intensity by default (#3527)#3529
brendancol merged 4 commits into
mainfrom
issue-3527

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3527

rate_of_spread returns spread rate in m/min, but fireline_intensity applied Byram's I = H*w*R as if R were in m/s. Chaining them gave fireline intensities 60x too high. The example already worked around this by dividing the spread rate by 60 before the call; a user chaining directly got no warning.

  • fireline_intensity now takes spread_rate_units, defaulting to 'm/min' so the output of rate_of_spread can be passed straight in. m/min inputs are divided by 60 before Byram's equation; pass 'm/s' for SI spread rates.
  • Docstrings on both functions now state their units and cross-reference each other.
  • Dropped the manual /60 conversion in examples/fire_propagation.py and the user-guide notebook.

Backend coverage: the unit conversion is a scalar operation on the spread DataArray's .data before dispatch, so it works the same on numpy, cupy, dask+numpy, and dask+cupy. The existing cross-backend parity tests are unaffected (the factor applies uniformly).

Test plan:

  • m/min default divides by 60 (60 m/min == 1 m/s)
  • explicit m/s preserves I = HwR
  • m/min result == m/s result / 60
  • invalid units string raises ValueError
  • chaining rate_of_spread -> fireline_intensity matches the explicit m/min call
  • full test_fire.py: 77 passed locally (numpy, cupy, dask+numpy, dask+cupy)

rate_of_spread returns m/min, but fireline_intensity applied Byram's
equation as if the rate were m/s, so chaining the two produced
intensities 60x too high. Add a spread_rate_units parameter ('m/min'
default to match rate_of_spread, or 'm/s') and convert to m/s before
applying Byram's equation. Update the docstrings, the example, and the
user-guide notebook to drop the manual /60 workaround.

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PR Review: Accept m/min spread rate in fireline_intensity by default

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • Backward-compat visibility: the new default flips existing callers who passed an m/s spread rate without naming the unit; their results now come out 60x smaller. The choice is intentional, but it is a silent behavior change, so it should be called out in the release notes when the changelog is updated at release time (the changelog is deliberately untouched here). xrspatial/fire.py:489

Nits (optional improvements)

  • The equation line in the docstring still reads I = H * w * R ... *R* is rate of spread (m/s) (xrspatial/fire.py:492-493). The paragraph right below resolves it (the equation wants m/s; the input defaults to m/min), but a reader skimming only the formula line could be briefly confused. Optional wording tweak.
  • The unit-conversion math is asserted directly only on the numpy backend (test_default_units_are_m_per_min, test_m_per_min_is_sixty_times_m_per_s). Cross-backend coverage comes transitively: the existing numpy/cupy/dask parity tests now run with the m/min default, so the /60 path is exercised on all four backends. Adequate as-is; noting for the record.

What looks good

  • The conversion is a single scalar divide on spread_rate_agg.data before dispatch, so it stays lazy on dask and backend-agnostic across numpy/cupy/dask+cupy. No premature materialization.
  • NaN-safe: NaN/60 stays NaN and the kernel's r != r guard still fires.
  • spread_rate_units is validated with a clear ValueError; docstrings on both functions now state units and cross-reference each other.
  • The manual /60 workaround is removed from both the example and the user-guide notebook, so the docs now demonstrate the corrected chaining.
  • Full test_fire.py passes locally (77 tests, all four backends on this CUDA box).

Checklist

  • Algorithm matches reference (Byram I = HwR needs R in m/s; /60 from m/min is correct)
  • All implemented backends produce consistent results (parity tests pass under new default)
  • NaN handling is correct
  • Edge cases covered (NaN propagation, invalid-units ValueError, chaining)
  • Dask chunk boundaries handled (scalar op preserves chunking)
  • No premature materialization or unnecessary copies
  • Benchmark not needed (no perf-critical change)
  • README feature matrix not applicable (no new function, no backend change)
  • Docstrings present and accurate

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Follow-up review (after 9171d53)

The docstring nit from the first pass is addressed: the equation line now reads *R* is rate of spread in m/s (see spread_rate_units ...), so the formula no longer reads as contradicting the m/min input default (xrspatial/fire.py:492-494).

Re-ran test_fire.py::TestFirelineIntensity after the change: 11 passed.

Remaining items are non-code and intentional:

  • Release-notes call-out of the default unit change: deferred to release time (the changelog is deliberately untouched during this workflow).
  • Direct unit-conversion assertions live on the numpy backend; the other three backends cover the /60 path transitively through the existing parity tests, which now run under the m/min default. Adequate as-is.

No blockers. Recommend merge once CI is green.

@brendancol brendancol merged commit ca5e964 into main Jun 26, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fireline_intensity expects m/s but rate_of_spread returns m/min; chaining gives 60x intensity

1 participant