EcologyR / mappestRisk

Other
1 stars 0 forks source link

Rework on ìnteractive_map()` #57

Closed dario-ssm closed 2 weeks ago

dario-ssm commented 2 months ago

The function might need to incorporate the possibility to plot either the estimate (i.e., the average number of months per year based on the tavg values of the 13th raster layer output from map_risk(), and the sd as proxy of uncertainty map obtained from that same raster using the sd values layer.

Additionally, we may consider an argument for the user to decide whether an interactive, leaflet map or a static (e.g., ggplot) map should be returned. This may not be necessary for now.

Pakillo commented 1 month ago

Just a comment: for easiest switching between static and interactive (leaflet) maps, I think the optimal way is using tmap: https://r-tmap.github.io/tmap/articles/tmap-getstarted.html#interactive-maps. But of course one could instead write specific functions for ggplot and leaflet; it's just more coding.

I think map_risk could be a generic mapping function, with an argument 'interactive' to set the output map as interactive or static. That way we could probably stick with terra (base) plots and leaflet, no?

AMBarbosa commented 1 month ago

If you don't require lots of customization, I can easily add the 'interactive' argument to 'map_risk' and make it plot either a static or an interactive map, besides outputting the rasters.

AMBarbosa commented 1 month ago

I've added the plot and interactive arguments to map_risk() (please pull), with a simple implementation based on terra::plet() (which uses leaflet). Users may want to set plot to FALSE to make the function run faster, especially for large datasets.

These arguments may render the interactive_map() function obsolete. Maybe you can re-assign @ajpelu to work on the tests of map_risk() instead of that? The tests would take me much longer to work on, as I have very limited experience with testthat.

Pakillo commented 1 month ago

Excellent! I didn't know about plet! :raised_hands:

dario-ssm commented 1 month ago

Thanks, it seems really nice!

I have changed the color scale (adding a new dependency, see the commit message) and minor things. Sometimes the viridis (default) scale seems a bit confusing to me when combining "risk" with "heat", so i tried a colorblind-friendly palette from khroma package. Feel free to discard these changes if they don't convince you Marcia. Thank you for the effort. I also think Antonio may assign the testing work.

dario-ssm commented 1 month ago

As Marcia said, I also think that ìnteractive_plot()` may be let obsolete.

We mighet still incorporate few things. First, mapping the sd values layer as an argument uncertainty_map = TRUE if desired by the user, as well as averaging the bootstrapped thermal bounds to the projection at each cell

AMBarbosa commented 1 month ago

OK, but in map_risk.R it looks like lines 205 and 214 repeat the same command. I would move it to line 198 (just after if (verbose) cat("\nPlotting map...\n")) to use only once.

dario-ssm commented 1 month ago

Thanks! I have just changed it, as well as the palette object name

dario-ssm commented 2 weeks ago

I have solved the color scales for plotting the two variables with both terra::plot() and terra::plet(). The former allows using two different color palettes, but the latter does not allow it unless we change the function to a leaflet() call rather than terra::plet(). For now, I suggest to remain plet().