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]: Adjust Ignitions to use `SF` instead of `vect` #45

Closed BadgerOnABike closed 2 months ago

BadgerOnABike commented 2 months ago

Contact Details

brett.moore@nrcan-rncan.gc.ca

What happened?

Terra is not handling the point transformation from raster appropriately. It consistently dropped the first point, we are going to move to SF. Further on initial testing on the glacier scenario a time savings of 50% was found. Not sure how this scales with the new method but we will endeavor to test that!

I will also be looking for any other opportunities to improve performance by moving to SF for point manipulation.

Relevant fix:


    data.frame %>%
    st_as_sf(coords=c("x","y"), crs=st_crs(fuelsRaster)) %>%
    st_transform("EPSG:4326") %>%
    st_coordinates```

### What component are you seeing the problem on?

R

### Relevant log output

_No response_

### Approvals Process

- [ ] Testing For Issue
- [ ] Merge
shreeramsenthi commented 2 months ago

There are similar pieces in the fire growth transformers: https://github.com/BurnP3/BurnP3PlusPrometheus/blob/cf42b4cfed868330e9a1eda257dc4da6165a2997/src/growth-pandora.R#L394 If it really is that much faster, it could make a significant difference to the overall run time.

We had previous issues with loading the raster package in the prometheus fire growth transformer since it modified the GDAL path (or maybe it was proj?) which made Prometheus silently fail and not grow any fires. I don't know if sf does anything of the sort, but that would be something we need to test if we want to use it in BP3+Prometheus.

BadgerOnABike commented 2 months ago

From conditions.R


> system.time(cellFromLatLong(
                       fuelsRaster,
                       lat = DeterministicIgnitionLocation$Latitude, 
                       long = DeterministicIgnitionLocation$Longitude))   
user  system elapsed 
0.16    0.08    0.43 

Warning message:
PROJ: Cannot open https://cdn.proj.org/ca_nrc_ABCSRSV4.tif: schannel: CertGetCertificateChain trust error CERT_TRUST_IS_UNTRUSTED_ROOT (GDAL error 1) 

>  system.time(cellFromXY(fuelsRaster,xy = st_as_sf(data.frame(long=DeterministicIgnitionLocation$Longitude,                                                                                           
                                                            lat=DeterministicIgnitionLocation$Latitude),
                                                            crs = "EPSG:4326",
                                                            coords = c("long","lat")) %>%
                                                            st_transform(crs = crs(fuelsRaster)) %>%
                                                            st_coordinates))   
user  system elapsed 
 0.04    0.00    0.05 ```
Also the error goes away moving to SF

Similar savings have been seen in `ignitions.R` in two locations.
BadgerOnABike commented 2 months ago

There are similar pieces in the fire growth transformers: https://github.com/BurnP3/BurnP3PlusPrometheus/blob/cf42b4cfed868330e9a1eda257dc4da6165a2997/src/growth-pandora.R#L394 If it really is that much faster, it could make a significant difference to the overall run time.

We had previous issues with loading the raster package in the prometheus fire growth transformer since it modified the GDAL path (or maybe it was proj?) which made Prometheus silently fail and not grow any fires. I don't know if sf does anything of the sort, but that would be something we need to test if we want to use it in BP3+Prometheus.

Noted. I also got rid of the custom cellfromLatLong function as we can use cellFromXY with some sf manip on the way, for a massive process time savings.

shreeramsenthi commented 2 months ago

Sounds good!

BadgerOnABike commented 2 months ago

Low level testing:

Cell2Fire example ignitions.R old 48s - new 49s (negligible difference, likely to be more noticeable in favor of the new with a larger sample, testing will continue)

conditions.R old 61s - new 50s