Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds first-class Sankey diagram support to the R rtemis.draw package (new S7 option classes + a draw_sankey() helper), while also refactoring a large set of option/property validators to rely on shared helpers (e.g., optional_* scalar properties and enum()), and making a small behavior change to draw_spectrogram() margins.
Changes:
- Add Sankey S7 classes (
SankeyNodeItem,SankeyEdgeItem,SankeyLevelOption,SankeySeries) plusdraw_sankey()and documentation/tests. - Refactor many S7 property definitions to use
optional_character_scalar,optional_logical_scalar, andenum(); updatedrop_nulls()to drop zero-length prototype values. - Update
draw_spectrogram()default margin behavior (now auto-derived whenmargins = NULL) and modernize Makefile dev targets.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| r/R/draw.R | Adds draw_sankey() and swaps some inline validations for shared check_* helpers. |
| r/R/series_sankey.R | Introduces Sankey-related S7 classes and Sankey series serialization logic. |
| r/tests/testthat/test-series_sankey.R | Adds unit tests covering Sankey classes and draw_sankey(). |
| r/R/utils.R | Updates drop_nulls() behavior and removes older helper property constructors. |
| r/R/styles.R | Replaces color_property/enum_property usage with optional_* scalars and enum(). |
| r/R/theme.R | Updates theme properties to use optional_character_scalar for background color. |
| r/R/series.R | Updates many series properties to use optional_* scalars and enum(). |
| r/R/option.R | Updates global option properties (e.g., animation, background_color) to new validators. |
| r/R/label.R | Updates label-related boolean properties to optional_logical_scalar. |
| r/R/components.R | Updates components’ properties to new optional_* and enum() helpers. |
| r/R/axis.R | Updates axis properties to new optional_* and enum() helpers. |
| r/R/draw_spectrogram.R | Changes margins default to NULL and adds auto-margin logic. |
| r/NAMESPACE | Exports new Sankey classes and draw_sankey(). |
| r/DESCRIPTION | Bumps version, adds rtemis.core (>= 0.1.0), and collates series_sankey.R. |
| r/man/draw_sankey.Rd | Adds generated documentation for draw_sankey(). |
| r/man/SankeySeries.Rd | Adds generated documentation for SankeySeries. |
| r/man/SankeyNodeItem.Rd | Adds generated documentation for SankeyNodeItem. |
| r/man/SankeyEdgeItem.Rd | Adds generated documentation for SankeyEdgeItem. |
| r/man/SankeyLevelOption.Rd | Adds generated documentation for SankeyLevelOption. |
| r/man/to_json.Rd | Fixes Rd link target for jsonlite::toJSON(). |
| r/man/rtemis.draw-package.Rd | Updates package Rd author section formatting. |
| r/man/rtemis_colors.Rd | Updates generated Rd formatting for rtemis_colors. |
| r/man/draw_spectrogram.Rd | Reflects the margins = NULL signature change. |
| Makefile | Expands dev targets (format/document/test/build/check/site/clean) and standardizes output. |
| demo_spectrogram.csv | Adds demo CSV data at repo root. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| required_cols <- c("source", "target", "value") | ||
| missing_cols <- setdiff(required_cols, names(links)) | ||
| if (length(missing_cols) > 0L) { | ||
| cli::cli_abort( | ||
| "{.arg links} must have columns {.val {required_cols}}; missing: {.val {missing_cols}}." | ||
| ) | ||
| } |
| # ECharts ignores a palette longer than the node count; trim (or cycle) to | ||
| # exactly one entry per node. | ||
| palette <- rep_len(color %||% rtemis_colors, length(node_names)) | ||
| opt <- EChartsOption( | ||
| title = if (!is.null(title)) Title(text = title, left = "center") else NULL, | ||
| tooltip = Tooltip(trigger = "item"), | ||
| color = palette, | ||
| series = SankeySeries( |
| properties = list( | ||
| name = optional_character_scalar, | ||
| value = numeric_or_null_property(), | ||
| depth = numeric_or_null_property(), |
| validator = function(value) { | ||
| if (is.null(value)) { | ||
| return("depth is required and must be a non-negative integer") | ||
| } | ||
| if (is.numeric(value) && length(value) == 1L && value >= 0) { | ||
| return(NULL) | ||
| } | ||
| "must be a non-negative number" | ||
| } |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the rtemis.draw package to version 0.2.1, introducing Sankey diagram support through new S7 classes and the draw_sankey function. It also refactors property definitions across the package to utilize updated S7 scalar and enum types, improves margin handling in spectrograms, and enhances the build system. Feedback highlights several improvement opportunities: ensuring node name derivation in draw_sankey handles factor columns correctly by coercing to character, refining property validators for Sankey sort and depth to align with ECharts specifications, and maintaining numeric type consistency when assigning default margins in spectrograms.
| } | ||
|
|
||
| # Derive unique node names from source and target columns | ||
| node_names <- unique(c(links[["source"]], links[["target"]])) |
There was a problem hiding this comment.
Deriving node names using c() on factor columns will result in an integer vector if the source and target columns have different levels. This will cause a mismatch between node names and link references in the resulting ECharts option. It is safer to coerce to character first.
node_names <- unique(c(as.character(links[["source"]]), as.character(links[["target"]])))| if (is.character(value) && length(value) == 1L && value == "desc") { | ||
| return(NULL) | ||
| } | ||
| "must be 'desc' or NULL" |
There was a problem hiding this comment.
The sort property in ECharts Sankey series supports 'ascending', 'descending', and 'none' (or null). The current validator only allows 'desc', which is likely not a valid value for ECharts and also prevents using other sorting modes.
if (is.character(value) && length(value) == 1L && value %in% c("ascending", "descending", "none")) {
return(NULL)
}
"must be 'ascending', 'descending', 'none', or NULL"| if (is.null(value)) { | ||
| return("depth is required and must be a non-negative integer") | ||
| } | ||
| if (is.numeric(value) && length(value) == 1L && value >= 0) { |
There was a problem hiding this comment.
The depth property in a Sankey diagram should be a non-negative integer. While is.numeric allows any number, it's better to ensure it's an integer to avoid layout issues in ECharts.
if (is.numeric(value) && length(value) == 1L && value >= 0 && value == as.integer(value)) {| margins[["right"]] <- "90" | ||
| if (!is.null(title)) { | ||
| margins[["top"]] <- "50" | ||
| } |
There was a problem hiding this comment.
Assigning string values to elements of the margins numeric vector will cause the entire vector to be coerced to character. It is better to use numeric values to maintain type consistency.
margins[["right"]] <- 90
if (!is.null(title)) {
margins[["top"]] <- 50
}
added sankey