GiottoVisuals 0.2.14 #122

jiajic · 2025-11-19T21:52:52Z

Summary by CodeRabbit

New Features
- Previously internal plotting helper is now publicly available.
- New show_axes option to toggle axis display across spatial plotting functions.
- Package version bumped to 0.2.14.
Bug Fixes
- Restored correct handling of coord_fix_ratio = TRUE after plotting library updates.
Changes
- Minimum required R version bumped to 4.5.0.
- Renamed a parameter in the spatial in‑situ points plotting API (breaking change).

_{✏️ Tip: You can customize this high-level summary in your review settings.}

make aes_string2() an exported function

replace deprecated readGiottoInstructions() -> instructions()

Make changes recommended by BiocCheck::BiocCheck()

harmonize a param and refactor

run devtools::document

coderabbitai · 2025-11-19T21:53:01Z

Caution

Review failed

The pull request is closed.

Walkthrough

Bumps package to 0.2.14 and R >= 4.5.0, exports aes_string2, adds show_axes across spatial plotting APIs, centralizes axis/aspect handling via .theme_remove_axes() and .aspect_ratio(), refactors spatInSituPlotPoints into per-element helpers with ordered assembly, plus docs/formatting edits.

Changes

Cohort / File(s)	Summary
Package metadata & exports `DESCRIPTION`, `NAMESPACE`, `NEWS.md`	Version -> 0.2.14; `R (>= 4.5.0)` required; `aes_string2` exported; release notes updated.
Exported helper promotion `R/aux_visuals.R`, `man/aes_string2.Rd`	`aes_string2` annotated `@export`; Rd internal keyword removed; implementation/signature unchanged.
Axis & aspect helpers `R/aux_visuals.R`, `R/gg_settings.R`	Added `.aspect_ratio()` (treats `coord_fix_ratio = TRUE` as `1` then applies `coord_fixed`) and `.theme_remove_axes()` (removes axes/ticks/titles/borders).
Spatial plotting — gg-based `R/vis_spatial_gg.R`, `man/spatPlot.Rd`, `man/spatFeatPlot2D.Rd`, `man/spatFeatPlot2D_single.Rd`, `man/dot-spatPlot2D_single.Rd`, ...	New `show_axes` parameter added to multiple spatial plotting functions; plots use centralized `.theme_remove_axes()` and `.aspect_ratio()`; docs updated.
Spatial in-situ plotting refactor `R/vis_spatial_in_situ.R`, `man/spatInSituPlotPoints.Rd`	`spatInSituPlotPoints` renamed parameter `spat_enr_names` → `spat_enr_name` and added `show_axes`; plotting refactored to `plot_order` with helpers (`.sissp_image`, `.sissp_polygon`, `.sissp_points`); axis/aspect centralized.
Color handling change `R/gg_info_layers.R`	Removed oob-squish clamping of numeric color values to `gradient_limits` when `color_as_factor = FALSE`.
Output/instructions & minor formatting `R/aux_output.R`, `R/aux_defaults.R`, `R/gg_annotation_raster.R`, `R/gg_param.R`, `R/plot_dotplot.R`, `R/plot_ridge.R`, `R/plot_sankey.R`	Mostly formatting and roxygen/doc tweaks; some `readGiottoInstructions(...)` argument layout changes and replacements with `instructions(gobject, ...)` in places.
Documentation updates many `man/*.Rd` (e.g., `man/combine_aes.Rd`, `man/ridgePlot.Rd`, `man/spatDeconvPlot.Rd`, `man/spatDimPlot.Rd`, `man/gg_param.Rd`, `man/plot_params.Rd`, ...)	Added/updated `show_axes` docs where applicable; removed `spat_enr_names` docs; multiple description wraps and example formatting adjustments.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant U as User
    participant API as spatInSituPlotPoints
    participant PL as Plot Assembler
    participant IMG as .sissp_image
    participant POL as .sissp_polygon
    participant PTS as .sissp_points
    participant THEME as .theme_remove_axes
    participant ASPECT as .aspect_ratio

    U->>API: call spatInSituPlotPoints(..., spat_enr_name, show_axes)
    API->>PL: compute plot_order & prepare args
    PL->>IMG: add image layer (if present)
    PL->>POL: add polygon layer (if present)
    PL->>PTS: add points layer (if present)
    PL->>THEME: if show_axes == FALSE -> remove axes
    PL->>ASPECT: apply aspect ratio handling
    PL->>API: return assembled ggplot
    API-->>U: ggplot object

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Areas needing extra attention:
- R/vis_spatial_in_situ.R — ensure plotting order/refactor preserves prior visual results and the spat_enr_name rename is consistent across callers/docs.
- R/aux_visuals.R & R/gg_settings.R — verify .aspect_ratio() and .theme_remove_axes() behavior matches previous coord/axis semantics.
- R/gg_info_layers.R — validate removal of numeric color clamping is intentional and acceptable for edge cases.
- NAMESPACE / man files — confirm aes_string2 export and Rd edits align.

Possibly related PRs

GiottoVisuals v0.2.13 #114 — Prior work touching aes_string2 implementation/visibility; closely related to this PR's promotion/export of aes_string2.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'GiottoVisuals 0.2.14' is vague and generic, serving only as a version identifier without describing the actual changes or improvements in the pull request.	Consider using a more descriptive title that highlights the main changes, such as 'Add show_axes parameter and fix coord_fix_ratio handling' or 'GiottoVisuals 0.2.14: improve axis control and coordinate fixing'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 81f12ab and 76b42cf.

📒 Files selected for processing (1)

R/vis_spatial_in_situ.R (10 hunks)

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

Provide your own instructions using the high_level_summary_instructions setting.
Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

📝 Description — Summarize the main change in 50–60 words, explaining what was done.

📓 References — List relevant issues, discussions, documentation, or related PRs.

📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.

📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |

✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)

man/ridgePlot.Rd (1)

98-100: Parameter name mismatch: scale_axis vs axis_scale

The usage block defines scale_axis, but the argument section documents axis_scale. This inconsistency can confuse users and tools relying on docs. Please align the argument name in the \item{} block with the actual parameter (scale_axis) and regenerate the Rd.
R/gg_annotation_raster.R (1)
136-155: Name collision between ext argument and ext() function will cause runtime error in default case

The parameter ext = NULL shadows the ext() function from terra. When .guess_plot_extent() is called without an ext argument (the default), control reaches line 145 and attempts to call ext(gobject, ...) where ext resolves to the NULL parameter, not the global function.

Per R's scoping rules, parameter names unconditionally mask identically named objects in outer scopes, and the actual call site at R/gg_info_layers.R:1473 does not supply an ext argument, confirming this code path is exercised.

The fix is to namespace the ext() function calls explicitly:
 .guess_plot_extent <- function(
         gobject, spat_unit = NULL, spat_loc_name = NULL, ext = NULL, ...) {
-    if (!is.null(ext)) ext <- ext(ext) # normalize to `SpatExtent` class
+    if (!is.null(ext)) ext <- terra::ext(ext) # normalize to `SpatExtent` class
     # if ext already given, directly return
     if (inherits(ext, "SpatExtent")) {
         return(ext)
     }
 
     # find extent from one of poly, spatlocs, points, in that order of pref
-    e <- ext(
+    e <- terra::ext(
         gobject,
         spat_unit = spat_unit,
         all_data = FALSE,
         verbose = FALSE,
         name = list(spatlocs = spat_loc_name),
         ...
     )
man/spatInSituPlotPoints.Rd (1)
216-223: Fix example argument name

The examples still pass spat_enr_names, but the function now accepts spat_enr_name. Running this example will throw unused argument 'spat_enr_names'. Please update the example call to match the new argument name.
-    spat_enr_names = "cluster_metagene",
+    spat_enr_name = "cluster_metagene",
R/vis_spatial_in_situ.R (1)

79-87: Example still uses spat_enr_names while the argument is now spat_enr_name

The public signature of spatInSituPlotPoints() takes spat_enr_name (Line 120), but the example above still calls the function with spat_enr_names = "cluster_metagene" (Lines 79–87). That mismatch will cause an “unused argument spat_enr_names” error when running examples.

Please update the example (and any other user-facing docs) to use spat_enr_name so that examples remain runnable and consistent with the API.

Also applies to: 120-121

🧹 Nitpick comments (9)

DESCRIPTION (1)

3-3: R version bump is correct, but CI runner must be updated

Requiring R (>= 4.5.0) is consistent with the 0.2.14 bump, but the current R-CMD-check job runs R 4.4.3, so installation fails before checks run. Consider updating the Actions workflow (or Bioc config) to use an R 4.5.x image so this PR can go green without loosening the dependency.

Also applies to: 26-26

R/aux_defaults.R (1)

429-431: Refactor of gradient helpers is formatting-only

The multi-line signatures for .evaluate_color_gradient_divergent() and .evaluate_color_gradient_sequential() preserve argument order and are called with named parameters, so there’s no behavioral change—just improved readability.

Also applies to: 476-478

man/spatDeconvPlot.Rd (1)

65-66: Minor wording nit in pie_scale description

To avoid the “mapping exists” grammar hiccup, consider rephrasing as e.g. “amount to scale the pie size if no radius mapping exists”. Purely a doc polish; behavior is unaffected.

man/spatDimPlot.Rd (1)

195-196: Rd line-wrapping changes only; semantics unchanged

The new line breaks for spat_center_point_border_stroke and dim_background_color keep the same parameter meaning and just improve readability. If you touch this file again, you might consider correcting “border strike size” → “border stroke size”, but that’s purely editorial.

Also applies to: 243-244

R/aux_output.R (1)

78-107: Commented-out helper and #. marker can be cleaned up or removed

The modified plot_output_handler_read() block is entirely commented out, and the #. prefix on continuation lines (e.g., Line 100) looks like an editing artifact. It has no runtime impact but may confuse future readers.

If this helper is not going to be revived soon, consider either deleting the block or cleaning up the comments (including the #. prefix) to keep the file tidy.

NEWS.md (1)

1-11: Consider adding a release date to the 0.2.14 header for consistency

Previous sections include a date in the header (e.g., 0.2.13 (2025/09/30)), while GiottoVisuals 0.2.14 currently omits it. Adding the date would keep the NEWS format consistent and make it easier to track releases chronologically.

R/gg_settings.R (1)

29-40: Axis-removal helper looks good; ensure unit() is properly imported

.theme_remove_axes() correctly strips axes and panel border and zeros out margins, which fits the new show_axes flow.

One small thing to double-check: unit() is used without namespace. Please ensure grid::unit is imported in NAMESPACE (or call grid::unit(...)) so this helper works without relying on accidental namespace leakage.

R/vis_spatial_in_situ.R (2)

145-155: New show_axes param and per-element plotting helpers look correct; consider guarding against empty feats

The refactor that:

Introduces show_axes with a default of TRUE via %null% (Line 213),

Builds a plot_order stack based on show_image, show_polygon, plot_last, and feats (Lines 215–225) with an early return when there is nothing to plot (Lines 226–229),

And delegates to .sissp_image(), .sissp_polygon(), and .sissp_points() (Lines 232–303 and 343–517),

is a nice cleanup and makes the rendering order explicit. The show_axes handling and subsequent .theme_remove_axes() call also align with the new helper.

One edge case: plot_order uses !is.null(feats) as the condition for adding "points", so feats = character(0) would still trigger the points branch and run .sissp_points(). Depending on how combineFeatureData() / combineFeatureOverlapData() behave with an empty feature set, that may yield confusing warnings or errors.

If empty feats is not meaningful, consider normalizing it to NULL up front or adding a length(feats) > 0 check when constructing plot_order to avoid surprising behavior.

Also applies to: 213-229, 232-303, 343-517

305-317: Centralized aspect ratio handling via .aspect_ratio() – verify behavior for coord_fix_ratio values

spatInSituPlotPoints(), .spatInSituPlotHex_single(), and .spatInSituPlotDensity_single() now all delegate coordinate ratio handling to .aspect_ratio(plot, coord_fix_ratio) instead of managing coord_fixed directly.

This centralization is good for consistency and matches the NEWS entry about restoring coord_fix_ratio = TRUE behavior, but it does shift responsibility to .aspect_ratio. Please verify that .aspect_ratio:

Correctly interprets coord_fix_ratio = TRUE vs numeric ratios vs NULL,

Plays well with downstream theme settings and xlim/ylim,

And preserves previous default behavior for existing callers.

If these semantics are already covered by tests, that’s sufficient; otherwise adding a small test matrix around typical coord_fix_ratio settings would be helpful.

Also applies to: 327-328, 627-629, 897-899

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1846978 and 23cf7a5.

📒 Files selected for processing (24)

DESCRIPTION (2 hunks)
NAMESPACE (1 hunks)
NEWS.md (1 hunks)
R/aux_defaults.R (2 hunks)
R/aux_output.R (1 hunks)
R/aux_visuals.R (4 hunks)
R/gg_annotation_raster.R (1 hunks)
R/gg_info_layers.R (0 hunks)
R/gg_param.R (2 hunks)
R/gg_settings.R (1 hunks)
R/plot_dotplot.R (2 hunks)
R/plot_ridge.R (2 hunks)
R/plot_sankey.R (1 hunks)
R/vis_spatial_gg.R (16 hunks)
R/vis_spatial_in_situ.R (9 hunks)
man/aes_string2.Rd (0 hunks)
man/combine_aes.Rd (1 hunks)
man/dot-spatInSituPlotDensity_single.Rd (1 hunks)
man/gg_param.Rd (1 hunks)
man/ridgePlot.Rd (2 hunks)
man/spatDeconvPlot.Rd (1 hunks)
man/spatDimPlot.Rd (2 hunks)
man/spatFeatPlot2D.Rd (1 hunks)
man/spatInSituPlotPoints.Rd (2 hunks)

💤 Files with no reviewable changes (2)

man/aes_string2.Rd
R/gg_info_layers.R

🧰 Additional context used

🪛 GitHub Actions: R-CMD-check-bioc

man/spatDimPlot.Rd