Remove R/rpy2 dependency; replace with cffdrs_py#5176
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5176 +/- ##
==========================================
- Coverage 68.80% 68.78% -0.02%
==========================================
Files 392 391 -1
Lines 16373 16296 -77
Branches 1846 1815 -31
==========================================
- Hits 11265 11210 -55
+ Misses 4527 4515 -12
+ Partials 581 571 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| FMC, SFC, PC, PDF, CC, CBH, ISI, output = "WSV") | ||
| WSV <- ifelse(GS > 0 & FFMC > 0, WSV0, WS) | ||
| Calculate the net effective windspeed (WSV). | ||
| WSV = Slopecalc(..., output="WSV") when GS > 0 and FFMC > 0, else WS. |
There was a problem hiding this comment.
:nit - Maybe remove this line of the comment, I don't think it's very helpful.
| fuel_type=fuel_type.value, | ||
| ffmc=ffmc, | ||
| bui=bui, | ||
| wsv=wsv if wsv is not None else 0, |
There was a problem hiding this comment.
I take it setting wsv = 0 here is equivalent to setting wsv = NULL in the R implementation?
There was a problem hiding this comment.
Nope, good catch, it should be required, changed here: 4d37183
| return buildup_index(dmc, dc) | ||
|
|
||
|
|
||
| def rate_of_spread_t(fuel_type: FuelTypeEnum, |
There was a problem hiding this comment.
With the R implementation we used to check the value being returned from the calculation and would raise a CFFDRSException if necessary. We're now returning the value without a check. Is this safe and intentional? We're doing this in a number of places now.
There was a problem hiding this comment.
The old pattern was defensive for a specific reason: the R functions could return non-numeric types (NA, NULL, wrong-length vectors) and rpy2 would just hand you that back as a Python object. So every call had:
if isinstance(result[0], float):
return result[0]
raise CFFDRSException("Failed to calculate X")
The new Python cffdrs library returns Python values directly, so that specific check is no longer needed. But I could add further input guards.
There was a problem hiding this comment.
I added more consistent input guards here: ee36112
I've left the behaviour for: fine_fuel_moisture_code, duff_moisture_code, drought_code, isi, fwi, bui_calc because:
morecast_v2/forecasts.py — fully defensive, guards every call with if yesterday.X is not None, stores results, checks them before downstream use. None propagation
is intentional and relied upon.
critical_hours.py and fwi_adjust.py — call without None checks, pass results directly into the next function. They depend on the return None contract silently
propagating through the chain: ffmc → isi → fwi, dmc/dc → bui → fwi. If any of these raised instead, those callers would crash.
Where does this happen? I think cffdrs_py still handles it internally |
| calc_step: bool = False, | ||
| batch: bool = True, | ||
| hourly_fwi: bool = False, |
There was a problem hiding this comment.
These 3 args aren't used. I actually don't think hourly_fine_fuel_moisture_code is used in our code base. Do we need it?
| raise CFFDRSException("Failed to calculate CFB") | ||
| csi = critical_surface_intensity(fmc, cbh) | ||
| rso = surface_fire_rate_of_spread(csi, sfc) | ||
| return _crown_fraction_burned(ros, rso) |
There was a problem hiding this comment.
crown fraction burned is calculated differently for the C6 fuel type. cffdrs_py has a different function for that calculation, wondering if we should take care of that in this PR
| return ws | ||
|
|
||
|
|
||
| def flank_rate_of_spread(ros: float, bros: float, lb: float): |
There was a problem hiding this comment.
I think functions like this might be unnecessary
There was a problem hiding this comment.
The value in these functions is that we encapsulate the underlying library, so it can change and we only need to make changes to this module.
There was a problem hiding this comment.
Oh I see what you mean, they are unused, removed here: cf41aa3
|
Took me awhile to figure out what was happening here, I was confused because R doesn't have fuel type default handling in those functions either, but it comes down to Python vs R math operations on
This is why C6 fuel type has built in 2 and 7m CBH on the front end Regardless, all the calculations in FireCalc look the same to me |
|
3 participants
Replaces all
R/rpy2calls inapp/fire_behaviour/cffdrs.pywith the pure-Pythoncffdrs_pypackage, eliminating the R runtime as a dependency.Changes:
cffdrs.pyto usecffdrs_pysubmodule importsr_importer.py, and CFFDRS pre-warming from startuprpy2frompyproject.toml; addedcffdrsgit dependency (pinned to commit)bui_calc, initial_spread_index, fire_weather_index, and crown_fraction_burned; fixed drought_code silently substitutingrh=0when RH is None; fixedl
ength_to_breadth_ratioreturning rather than raisingCFFDRSException; added per-row error handling tohourly_fine_fuel_moisture_codefba_calctests (removedCFFDRS.instance()pre-warming); deletedtest_r.pyBehavioural notes:
duff_moisture_code returns0.0 (not ~0.256) when temperature < 1.1°C —cffdrs_pyclamps temp to -1.1 before computing the drying rate (Eq. 16), zeroing it out. The R implementation did not apply this clamp.cbhuses a fuel type default if it isNone, this was handled internally before in the R library.Closes #1926
Test Links:
Landing Page
MoreCast
Percentile Calculator
C-Haines
FireCalc
FireCalc bookmark
Auto Spatial Advisory (ASA)
HFI Calculator
SFMS Insights
Fire Watch