search

cffdrs / cffdrs_py

Python Module for CFFDRS
GNU General Public License v2.0
6 stars 3 forks source link

implement rest of package #2

Open jordan-evens opened 1 year ago

conbrad commented 10 months ago

Any details on what's outstanding / priority? I'd be interested in contributing if you're looking for help.

jordan-evens commented 10 months ago

Hoping to implement FBP functions with names consistent with https://github.com/cffdrs/cffdrs_r/tree/jordan-evens/standardized-names - should just be a straight translation from R to python, at least initially. If you'd like to help that's awesome - thank you

gagreene commented 3 months ago

Hi there. I've written up a cffwis script that I'm happy to share (currently in a private repo). It accepts numpy arrays (for raster processing) and/or int/float values as inputs. I'm currently using it to calculate hourly and daily FWIS variables for ERA5-Land data for the entirety of Canada. I've done my best to use the variable naming conventions applied in the respective papers (e.g., Van Wagner 1977, 1987; etc.). Comparing my code with what you have, I'm noticing that I don't have the additional latitude adjustments for DMC and DC, but that would be simple enough to implement for numpy arrays.

I have also developed a CFFBPS class that I can share (also currently in a private repo). It's fully functional for int/float inputs, but will have to be revised to accept numpy arrays. I have to flesh out the function and parameter data type descriptions, but otherwise it's ready to go. This class is part of a CanPyro module that I'm developing in collaboration with Dan Perrakis. It's structurally very different from the cffwis script (and your current fwi script) since it uses inheritance, but might be worth checking out?

I'm also happy to contribute to the cffdrs_py module if you need coding assistance. I regularly ask Justin Beckers for data, and have probably taken up a month of his time in the past few years. This is the least I can do to show my gratitude.

gagreene commented 3 months ago

I should also mention that I found an error in the fwi() function. I forked the repo a few weeks ago and applied the fix (just missing a negative sign in front of 0.023 on line 424). Happy to open a new issue if you'd prefer.

jordan-evens commented 3 months ago

The reference implementation for this is the R package, and this seems the same as that: https://github.com/cffdrs/cffdrs_r/blob/main/R/fire_weather_index.r#L32 https://github.com/cffdrs/cffdrs_py/blob/main/cffdrs/fwi.py#L424

The paper at: https://cfs.nrcan.gc.ca/pubwarehouse/pdfs/19973.pdf

has: 1000 / (25 + 108.64 ( exp(-0.023 * BUI)

But it's division in the R package and this, not multiplication. I think: x ^ (-n) == 1 / (x ^ n), so the sign is correct when it's using division?

jordan-evens commented 3 months ago

@gagreene @RobJago It sounds like you both might have similar implementations for this being applied across arrays/rasters?

I'm hesitant to add object-orientation to this since it's supposed to be the simplest possible implementation of the point-based equations so people can follow them, focused on clarity of understanding and not necessarily speed. But I do think having functions that can be applied across arrays is good, so if you have something that applies simple point-based functions in a reasonable way I'm not against including it.

@RobJago I think you were just applying the functions? Maybe it's worth trying to get what you've done into this so we have a starting point for arrays that we can look at improving on?

RobJago commented 3 months ago

@jordan-evens @gagreene Yes, I have just a straight up non-object orientation using masked arrays (numpy.ma). It would be great to have QC on this code.

I don't have permissions to start a new repo here but I could add a file to the existing python repo. Let me know what works best.

gagreene commented 3 months ago

I sent you both invites as collaborators on my cffdrs repo. You can check out the cffwis script there. I also implemented masked arrays, but created the script as a multipurpose tool, so there isn't a need for a raster and a non-raster version. It also takes combinations of int/float and rasters.

gagreene commented 3 months ago

The reference implementation for this is the R package, and this seems the same as that: https://github.com/cffdrs/cffdrs_r/blob/main/R/fire_weather_index.r#L32 https://github.com/cffdrs/cffdrs_py/blob/main/cffdrs/fwi.py#L424

The paper at: https://cfs.nrcan.gc.ca/pubwarehouse/pdfs/19973.pdf

has: 1000 / (25 + 108.64 ( exp(-0.023 * BUI)

But it's division in the R package and this, not multiplication. I think: x ^ (-n) == 1 / (x ^ n), so the sign is correct when it's using division?

@jordan-evens Yeah, fair enough. I overlooked the divide in your equation. Please disregard my proposed edit!