[Burn-P3+ Bug]: Fuels restricted in all not caught in individual seasons.

BurnP3 / BurnP3Plus

A SyncroSim package to explore fire risk and susceptibility across a landscape.

https://burnp3.github.io/BurnP3Plus/

MIT License

7 stars 4 forks source link

[Burn-P3+ Bug]: Fuels restricted in all not caught in individual seasons. #28

Open BadgerOnABike opened 8 months ago

BadgerOnABike commented 8 months ago

Contact Details

brett.moore@nrcan-rncan.gc.ca

What happened?

When we assess the restricted fuels, the "All" season is its own season rather than applying to all seasons.

https://github.com/BurnP3/BurnP3Plus/blob/3baf6d32e6fad1b44b52c843e58f1f21ed438abb/src/ignitions.R#L222

What component are you seeing the problem on?

Relevant log output

restrictedFuels <- IgnitionRestriction %>%
filter(Season == season | is.na(Season), Cause == cause | is.na(Cause), FireZone == firezone | is.na(FireZone)) %>% 
pull(FuelType)

restrictedFuels
[1] "Matted Grass"

IgnitionRestriction
  Iteration Timestep                FuelType Season Cause FireZone
1        NA       NA                Hydrogen    All    NA       NA
2        NA       NA            Matted Grass Spring    NA       NA
3        NA       NA Scrunched-up newspapers Summer    NA       NA

Approvals Process

[ ] Testing For Issue
[ ] Merge

shreeramsenthi commented 8 months ago

good catch! I was mostly conceptualizing the "All" season as something for the back end to fill in to Season as a clearer alternative to "NA" in the output, but didn't consider the user filling in "All" as an explicit alternative to NA. This might not be the only place that has this issue.

I don't suppose you'd want to go as far as making Season a required field in more places and requiring that users specify "All" instead of leaving it blank?

BadgerOnABike commented 8 months ago

Perhaps we have it say all where blanks exist for season to make it explicit.

I agree that this may be a larger change than just the 1 spot I've noted it. I have some other testing to do to see what really needs to be fixed and the expected behaviours but those will wait until after the course.

BadgerOnABike commented 8 months ago

So far the issues uncovered are:

I will edit this as I find more

[ ] - We also don't want to sort on All... so we need to exclude it from the piping (not positive on how to do this)

ignitions.R

[x] - All is categorized as its own season
[x] - All being a season inhibits ignition grids across all seasons from being enabled

conditions.R

[x] - Fire duration could contain an All season call, but currently ignores it
[x] - Hours Burning table could contain All season, but currently ignores it
[x] - Local weather shouldn't have All... but arguably could, and currently ignores it

burnProbability.R Accumulator seems to work.