Add spectrogram

[egenn]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces a new draw_spectrogram function for interactive time-frequency visualization and updates the package version. It also renames the internal widget identifier from rtemis.draw to draw. My feedback highlights that this renaming is a breaking change for existing Shiny applications and suggests vectorizing the nested loop in draw_spectrogram to improve performance for large datasets.

[gemini-code-assist]

Renaming the widget from rtemis.draw to draw is a breaking change. Any existing Shiny applications or custom JavaScript code that relies on the rtemis.draw identifier will no longer function correctly. While this aligns the widget name with the internal filename, it should be explicitly noted in the documentation or release notes.

[gemini-code-assist]

The nested loop for creating data_list is inefficient for large spectrograms (e.g., near the 500,000 cell limit mentioned in the warning). Vectorizing this operation using a matrix and as.vector() is significantly faster and reduces memory overhead. jsonlite will correctly serialize the resulting list of vectors into the array-of-arrays format expected by ECharts.

  spec_clean <- spec
  spec_clean[!is.finite(spec_clean)] <- NA
  data_list <- cbind(
    rep(seq_len(n_time_disp) - 1L, each = n_freq_disp),
    rep(seq_len(n_freq_disp) - 1L, times = n_time_disp),
    as.vector(spec_clean)
  )
  data_list <- unname(split(data_list, seq_len(nrow(data_list))))

[Copilot]

Pull request overview

Adds a new tier-1 draw_spectrogram() chart to the rtemis.draw R package, generating an interactive ECharts heatmap from either a raw signal (STFT computed via signal::specgram()) or a pre-computed spectrogram matrix.

Changes:

• Introduces draw_spectrogram() with internal helpers for windowing and palettes (including a theme-aware diverging palette).
• Adds roxygen/Rd documentation and comprehensive testthat coverage for validation and output structure.
• Aligns the htmlwidgets binding name to "draw" and updates package metadata (version bump + new imports).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
r/R/draw_spectrogram.R	New spectrogram implementation + helpers + roxygen docs.
r/tests/testthat/test-draw_spectrogram.R	New tests covering helpers, validation, and ECharts option structure.
r/man/draw_spectrogram.Rd	Generated documentation for draw_spectrogram().
r/R/draw.R	Updates htmlwidgets widget name used by createWidget() and Shiny output.
r/inst/htmlwidgets/draw.js	Updates JS htmlwidgets registration name to match R.
r/NAMESPACE	Exports draw_spectrogram().
r/DESCRIPTION	Version bump, adds signal + viridisLite imports, collates new file.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Header comment says the ECharts heatmap uses continuous value axes, but the implementation below always uses category axes (and explicitly explains why). Update/remove this comment so it doesn't mislead future maintainers.

Suggested change

	# cartesian2d coordinate system with continuous value axes.
	# cartesian2d coordinate system with category axes.

[Copilot]

n_fft is coerced with as.integer() but isn't validated for being a length-1, non-NA numeric value. If a caller passes NA, NULL, or a non-numeric value, comparisons like n_fft < 2L can error (missing value where TRUE/FALSE needed). Add explicit scalar validation before coercion.

[Copilot]

With db = TRUE, the code applies log10(spec + eps) without ensuring spec is non-negative. For pre-computed real matrices that include negative values this yields NaN, and downstream max()/range() can produce invalid limits. Consider aborting early if any values are < 0 when db = TRUE, or otherwise sanitizing/defining the expected input domain.

[Copilot]

When zlim is NULL, range(spec, na.rm = TRUE) will return c(Inf, -Inf) if spec has no finite values (e.g., after db conversion producing all NaN). Add a check that there is at least one finite value in spec (or that zlim is finite) and error with a clear message otherwise.

Suggested change

	if (is.null(zlim)) {
	zlim <- range(spec, na.rm = TRUE)
	if (is.null(zlim)) {
	if (!any(is.finite(spec))) {
	cli::cli_abort(
	c(
	"Cannot determine colour-scale limits automatically because the spectrogram contains no finite values.",
	"i" = "Check the input signal or provide a finite {.arg zlim}."
	),
	call = NULL
	)
	}
	zlim <- range(spec, na.rm = TRUE)
	} else if (!is.numeric(zlim) || length(zlim) != 2L || any(!is.finite(zlim))) {
	cli::cli_abort(
	"{.arg zlim} must be a numeric vector of length 2 with finite values.",
	call = NULL
	)

[Copilot]

Docs for power say it is applied only when x is complex, but the code also uses power to choose 10 vs 20 log scaling when db = TRUE even for real (pre-computed) matrices. Either adjust the behavior or update the documentation to match the current logic.

Suggested change

	#' @param power Logical: Compute power spectrum (`TRUE`, `|S|^2`) or amplitude
	#' spectrum (`FALSE`, `|S|`). Applied only when `x` is complex.
	#' @param db Logical: Convert to dB. For power: `10 * log10()`; for amplitude:
	#' `20 * log10()`. Set `FALSE` when passing a pre-computed dB matrix.
	#' @param power Logical: Treat spectral values as power (`TRUE`, `|S|^2`) or
	#' amplitude (`FALSE`, `|S|`). When `x` is complex, this controls whether the
	#' STFT is converted to a power or amplitude spectrum. When `db = TRUE`, it
	#' also determines the dB scaling applied to real/pre-computed matrices:
	#' `10 * log10()` for power and `20 * log10()` for amplitude.
	#' @param db Logical: Convert to dB. Uses `10 * log10()` when `power = TRUE`
	#' and `20 * log10()` when `power = FALSE`. Set `FALSE` when passing a
	#' pre-computed dB matrix.

[Copilot]

This test is named/described as checking that diverging palettes "symmetrise zlim around 0", but it currently only asserts lengths. Since asymmetric zlim is symmetrized internally, you can strengthen the test by asserting the palettes (and their dark_variant attrs) are identical between the symmetric and asymmetric cases, or otherwise update the test description to match what’s actually asserted.

[Copilot]

is_raw_signal treats a complex vector as a raw signal and will pass it into signal::specgram(), which is meant for real numeric signals. Consider defining raw-signal input as is.numeric(x) && is.vector(x) && !is.matrix(x) (and erroring on complex vectors).

Suggested change

	# Distinguish raw signal (vector) from pre-computed matrix
	is_raw_signal <- !is.matrix(x) && is.vector(x)
	if (is.complex(x) && is.vector(x) && !is.matrix(x)) {
	cli::cli_abort(
	"{.arg x} must be a real numeric vector when supplied as a raw signal; \\
	complex vectors are not supported."
	)
	}
	# Distinguish raw signal (real numeric vector) from pre-computed matrix
	is_raw_signal <- is.numeric(x) && is.vector(x) && !is.matrix(x)

[Copilot]

overlap is coerced via as.integer() without validating type/NA. For non-numeric inputs (or NA), ovlp becomes NA and the range check won’t catch it, potentially passing NA into signal::specgram(). Validate overlap as a length-1, non-NA numeric/integer value (when non-NULL) before coercion and range checking.

[Copilot]

Roxygen docs: palette is documented as "Character or Numeric" and contains a duplicated article ("Accepts a a"). The implementation only accepts character palette names or character vectors of hex colours, so the docs should reflect that and fix the grammar (source roxygen, so the generated Rd stays in sync).