Added predicted-probability mapping - #433
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved mapping, namespace, CRS, validation, security, and error-handling issues block approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds predicted-probability mapping for FishSET models, including static and interactive maps integrated into the Model Fit Shiny module.
Changes:
- Adds probability aggregation, spatial joining, map generation, and export logic.
- Adds probability-matrix saving and Shiny mapping controls.
- Adds unit tests for validation, calculations, and map outputs.
File summaries
| File | Description |
|---|---|
tests/testthat/test-map_predicted_probs.R |
Tests mapping validation, calculations, and outputs. |
R/map_predicted_probs.R |
Implements predicted-probability mapping and rendering. |
inst/ShinyFiles/MainApp/modules/model_fit_module.R |
Adds probability saving and Shiny mapping workflows. |
Review details
Suppressed comments (7)
R/map_predicted_probs.R:127
- This check only catches a mismatch when every spatial row has an
NAprobability. If at least one zone matches, model zones absent from the spatial data are silently removed by line 130 while the returned table still contains them, so the map and table disagree. Validate missing model zone IDs before filtering.
if (all(is.na(spat_join[[val_var]]))) {
stop("All joined probability values are NA. This means the Zone IDs in the model did
not match any Zone IDs in the spatial dataset.")
}
R/map_predicted_probs.R:166
map_data()expects longitude/latitude degrees, but these limits come fromst_bbox(spatdat)in whatever CRS the input uses. For projectedsfinputs, the limits are in metres (or another projected unit), so the coastline layer is empty or misaligned with the polygons. Generate the base map in WGS84 and transform/plot it in a consistent CRS.
ggplot2::map_data(map = map_name,
xlim = x_limits,
ylim = c(bbox["ymin"], bbox["ymax"]))
R/map_predicted_probs.R:104
- The fallback to
1:ncol()does not actually use spatial row order: the table is later joined by zone-ID value. For an unnamed probability matrix and ordinary IDs such asZone_A, every join value is unmatched and the function errors; require column names matching the spatial zone IDs or build a validated mapping explicitly.
zone_names <- colnames(fit$prob_matrix)
if (is.null(zone_names)) {
warning("prob_matrix lacks column names. Assuming column index matches zone ID order.")
zone_names <- as.character(1:ncol(fit$prob_matrix))
}
R/map_predicted_probs.R:262
- The documented
plot_typevalues are"dynamic"and"static", but any other value silently takes the static branch. A typo or invalid API value therefore produces a different output instead of a validation error; reject unsupported values before dispatching.
if (plot_type == "dynamic") {
z_plot <- z_plot_fun_dynamic(spat_join, legend_name = legend_name)
} else {
z_plot <- suppressWarnings(z_plot_fun_static(spat_join, legend_name = legend_name))
inst/ShinyFiles/MainApp/modules/model_fit_module.R:465
- The
unserialize_table()call runs before thetryCatchthat starts at line 473. A missing or corrupt ModelFit table therefore escapes this observer and does not produce the promised user-facing mapping failure notification. Put this lookup and fit check inside the same error-handling path.
full_fit_list <- unserialize_table(paste0(project_name, "ModelFit"), project_name)
fit <- full_fit_list[[input$map_fit_input]]
if (is.null(fit$prob_matrix)) {
inst/ShinyFiles/MainApp/modules/model_fit_module.R:544
- The static-map precheck has the same unhandled database-read failure:
unserialize_table()is outside thetryCatchbelow, so a missing or corrupt ModelFit table breaks the observer instead of notifying the user. Move this lookup and fit check into that error-handling path as well.
full_fit_list <- unserialize_table(paste0(project_name, "ModelFit"), project_name)
fit <- full_fit_list[[input$map_fit_input]]
if (is.null(fit$prob_matrix)) {
inst/ShinyFiles/MainApp/modules/model_fit_module.R:656
- The tooltip text contains the grammatical typo
mapping.:, which is shown directly in the UI. Remove the extra colon so the instruction reads naturally.
"Check this to enable spatial mapping.: Doing this for models with
massive datasets can cause memory constraints or slow down
- Files reviewed: 3/3 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| domain = spatdat[[var_sym]] | ||
| ) | ||
|
|
||
| fill_colors <- pal(spat_join[[var_sym]]) |
| hover_labels <- lapply( | ||
| sprintf( | ||
| "<strong>Zone ID:</strong> %s<br/><strong>Probability:</strong> %s", | ||
| spatdat[[zone_spat]], |
| #' @param output Output a `"plot"`, `"table"`, or both (`"tab_plot"`). | ||
| #' Defaults to `"plot"`. | ||
| #' | ||
| #' @export |
|
|
||
| # use WGS 84 if crs is missing | ||
| if (is.na(sf::st_crs(spatdat))) { | ||
| spat_join <- sf::st_transform(spat_join, crs = 4326) |
| ggplot2::coord_sf(xlim = c(bbox[1], bbox[3]), ylim = c(bbox[2], bbox[4]), | ||
| expand = TRUE) + | ||
| fishset_theme() + | ||
| ggplot2::theme(legend.key.size = unit(1, "cm"), |
Summary
Adds predicted-probability mapping for fitted FishSET models, including interactive Leaflet maps and exportable static ggplot maps.
What changed
map_predicted_probs()to calculate mean or observation-level predicted probabilities by zone, join them to spatial data, and save the resulting table and plot.Notes
Mapping requires models to be fitted with Save Full Probability Matrix enabled. This can increase memory usage for large datasets.