Translate all remaining R functionality#13
Conversation
| @pytest.mark.xfail( | ||
| reason="I believe default CFL should be 0, not 1 as it's currently written - https://github.com/cffdrs/cffdrs_r/blob/4e40dd3af841f3a708abd83d7f2d43fefb08649a/R/fire_behaviour_prediction.r#L38" | ||
| ) |
There was a problem hiding this comment.
I think it's possible there's a minor mistake in the fbp implementation and test data from the R package, that only surfaces if no input is provided
There was a problem hiding this comment.
It is strange as it sets the CFL in the input definition but then re-establishes later on based on the fuel type? So a C-2 would have a CFL of .8 once the crown_fuel_load function runs. It will run if CFL is between 0 and 2, but having a fueltype defined would overwrite? Strange behaviour imo.
| dir = bearingT1T2 - ThetaAdeg | ||
| if dir is not None: | ||
| if dir < -180: | ||
| dir += 360 |
There was a problem hiding this comment.
Are these + 360 and -360 correct? The R implementation has + 10 and -10.
There was a problem hiding this comment.
Good catch, you're right, this implementation is slightly different than the R implementation. I was a bit confused by the +/- 10 and in my conversations with a higher power it also thought the +/- 10 was a bug. However, a scenario where these implementations differ never came up in the tests for pros/lros where this function is used (there are no unit tests for direction in the R library).
I just generated some random inputs and the majority of them are the same:
Python outputs
Test 1: bearingT1T2=10, bearingT1T3=5, ThetaAdeg=20 -> dir=-10
Test 2: bearingT1T2=5, bearingT1T3=10, ThetaAdeg=20 -> dir=25
Test 3: bearingT1T2=-10, bearingT1T3=-5, ThetaAdeg=20 -> dir=10
Test 4: bearingT1T2=-5, bearingT1T3=-10, ThetaAdeg=20 -> dir=-25
Test 5: bearingT1T2=45, bearingT1T3=-45, ThetaAdeg=30 -> dir=15
Test 6: bearingT1T2=-45, bearingT1T3=45, ThetaAdeg=30 -> dir=-15
Test 7: bearingT1T2=100, bearingT1T3=-100, ThetaAdeg=50 -> dir=150
Test 8: bearingT1T2=100, bearingT1T3=-100, ThetaAdeg=90 -> dir=-170
Test 9: bearingT1T2=-100, bearingT1T3=100, ThetaAdeg=50 -> dir=-150
Test 10: bearingT1T2=-100, bearingT1T3=100, ThetaAdeg=90 -> dir=170
Test 11: bearingT1T2=10, bearingT1T3=5, ThetaAdeg=200 -> dir=170
Test 12: bearingT1T2=5, bearingT1T3=10, ThetaAdeg=200 -> dir=-155
Test 13: bearingT1T2=120, bearingT1T3=-120, ThetaAdeg=70 -> dir=-170
Test 14: bearingT1T2=-120, bearingT1T3=120, ThetaAdeg=70 -> dir=170
R outputs
Test 1: bearingT1T2=10.0, bearingT1T3=5.0, ThetaAdeg=20.0 -> DIR=-10.0
Test 2: bearingT1T2=5.0, bearingT1T3=10.0, ThetaAdeg=20.0 -> DIR=25.0
Test 3: bearingT1T2=-10.0, bearingT1T3=-5.0, ThetaAdeg=20.0 -> DIR=10.0
Test 4: bearingT1T2=-5.0, bearingT1T3=-10.0, ThetaAdeg=20.0 -> DIR=-25.0
Test 5: bearingT1T2=45.0, bearingT1T3=-45.0, ThetaAdeg=30.0 -> DIR=15.0
Test 6: bearingT1T2=-45.0, bearingT1T3=45.0, ThetaAdeg=30.0 -> DIR=-15.0
Test 7: bearingT1T2=100.0, bearingT1T3=-100.0, ThetaAdeg=50.0 -> DIR=150.0
Test 8: bearingT1T2=100.0, bearingT1T3=-100.0, ThetaAdeg=90.0 -> DIR=-170.0
Test 9: bearingT1T2=-100.0, bearingT1T3=100.0, ThetaAdeg=50.0 -> DIR=-150.0
Test 10: bearingT1T2=-100.0, bearingT1T3=100.0, ThetaAdeg=90.0 -> DIR=170.0
Test 11: bearingT1T2=10.0, bearingT1T3=5.0, ThetaAdeg=200.0 -> DIR=-180.0
Test 12: bearingT1T2=5.0, bearingT1T3=10.0, ThetaAdeg=200.0 -> DIR=195.0
Test 13: bearingT1T2=120.0, bearingT1T3=-120.0, ThetaAdeg=70.0 -> DIR=-170.0
Test 14: bearingT1T2=-120.0, bearingT1T3=120.0, ThetaAdeg=70.0 -> DIR=170.0
Test 11 & 12 is where they differ, but I actually think the Python implementation is correct. For test 12, the direction would be calculated as 205, then adjusted to 195, which I don't quite understand.
Whereas in Python the output is -155 (equivalent to 205 on a 360-degree circle)
This may be a question for the CFS devs, but wanted to write all this out because I forgot to revisit it
There was a problem hiding this comment.
An adjustment of 10 degrees does seem odd. At first glance it would seem that you would want to normalize the calculated direction to fit within 0-360 but neither implementation does that.
There was a problem hiding this comment.
Looks like the original paper normalizes to -180 to 180, so your Python implementation is correct I'd say. Maybe you could include something in the doc string to clarify this?
There was a problem hiding this comment.
Thanks for looking into the paper! Didn't realize it was in there. Good call, I'll put a note in
There was a problem hiding this comment.
@jordan-evens I think this would be more in your wheelhouse to discuss than mine, do you have anything to add here?
|
@BadgerOnABike @jordan-evens This is quite a large PR, but it contains all the remaining CFFDRS R code translation, along with all tests ported over from the R repo. Tried to maintain all the same comments and references to the source formulas just like the R library. |
| Row 77: Calculated SDMC=0.0484, Expected=0.0484, Diff=-0.0000 | ||
| Row 78: Calculated SDMC=5.9601, Expected=0.0484, Diff=5.9117 | ||
| Row 79: Calculated SDMC=19.1639, Expected=3.0500, Diff=16.1139 | ||
| Row 80: Calculated SDMC=32.3677, Expected=9.7560, Diff=22.6117""" |
There was a problem hiding this comment.
Maybe there's something I don't quite understand about how the R package is loading that data for tests
|
A few comments and I reopened one of the comments to get the table in the comment. Otherwise this is a huge lift well done folks, apologies for the delays! |
|
Worth keeping the discussion going to see if we need to adjust anything but I'd like to unblock further progress on this. |
This PR is a comprehensive conversion of the CFFDRS (Canadian Forest Fire Danger Rating System) R package to Python.
Key Changes
New Modules
cffdrs/models.py: Dataclasses for FBP inputs and outputs (FBPInput,FBPPrimaryOutput,FBPSecondaryOutput,FBPAllOutput)cffdrs/constants.py- Fuel type constantscffdrs/r_helpers.py- R compatibility helpersTest Suite
slope_calc.py,sdmc.pyand a singlefbptest. These failing tests have reasoning in their@pytestdecorators.slope_calc.pytest failures are edge case failures seemingly from some difference between R and Python mathematical operations, but shouldn't have a practical effect on it's outputs (all tests that use theslope_adjustmentfunction pass). The failures all occur whenWSVis very small, <1e-11@pytest.mark.xfailand run the testfbptest, I believe it might be due to a small error in the R code/test dataConfiguration Updates
pyproject.toml: Addedrufffor consistent code formatting.R compatibility
r_helpers.pyprovides R-style math operations for specific cases like div by 0