Skip to content

Add unit tests for the plotting functions; fix scales >= 1.3.0 compatibility - #49

Merged
zeehio merged 1 commit into
masterfrom
claude/gcims-plot-coverage
Jul 12, 2026
Merged

Add unit tests for the plotting functions; fix scales >= 1.3.0 compatibility#49
zeehio merged 1 commit into
masterfrom
claude/gcims-plot-coverage

Conversation

@zeehio

@zeehio zeehio commented Jul 12, 2026

Copy link
Copy Markdown
Member

Summary

None of this package's plot functions render to a graphics device — they build and return a ggplot object (or, for overlay_peaklist(), a list of ggplot2 layers) via + composition. That means they can be unit-tested like any other data structure, by inspecting the returned object's data/labels/scales/layer classes, without snapshot/visual-regression tooling (e.g. vdiffr).

  • mat_to_nativeRaster() (utils-plot.R): pure matrix→raster encoding — shape/class/channels attributes, determinism, character vs. pre-encoded colormap, and the zero-length edge case
  • plot_interactive(): wraps a ggplot in plotly::ggplotly(), checked it returns a "plotly" object
  • cubic_root_trans(): transform/inverse round-trip (including negative values, since it's a signed cube root) and the breaks() empty/non-finite edge case
  • as.data.frame.GCIMSSample(): melts the intensity matrix, checked columns/values, dt_range/row.names
  • plot() on GCIMSSample/GCIMSChromatogram/GCIMSSpectrum: labels, data, axis-range restriction, the remove_baseline branch, and the single-value/multi-value/empty-value subtitle branches
  • overlay_peaklist(): the color_by (none/literal color/column), apex, >10-groups-hides-legend, palette-recycling, non-data.frame input, and pdata-merging branches, plus its two validation errors

Brings plot-GCIMSSample.R, plot-GCIMSChromatogram.R, plot-GCIMSSpectrum.R, and utils-plot.R to 100% line coverage. Overall package coverage: 78.34% → 86.83%.

Bug fix

While testing plot(sample, trans = ...), found that mat_to_gplot() checked inherits(trans, "trans") to validate a transform object passed directly (not as a string). scales >= 1.3.0 renamed this S3 class from "trans" to "transform", so passing any transform object this way — including the package's own cubic_root_trans() — always failed with "unknown trans value", even though the roxygen docs explicitly point to this as valid usage (plot(s, trans = scales::identity_trans()), plot(s, trans = cubic_root_trans())).

Fixed the check to use the class name scales has used since 1.3.0 (already over a year old), and bumped DESCRIPTION's scales dependency to >= 1.4.0 accordingly. The string-based trans = "log10" path was unaffected (it goes through a different code path using the still-preserved legacy *_trans() function names).

Test plan

  • Ran each new test file individually — all pass
  • Full suite testthat::test_local(".") — 672 passing, 1 pre-existing skip, 0 failures
  • covr::package_coverage() confirms all four files at 100%; overall package coverage 78.34% → 86.83%
  • Manually verified the trans= bug end-to-end before and after the fix (string values, transform objects, and the two error paths)

Generated by Claude Code

…ibility

None of the package's plot functions render to a graphics device -- they
build and return a ggplot object (or, for overlay_peaklist(), a list of
ggplot2 layers) via + composition, so they can be unit-tested like any
other data structure by inspecting the returned object's data, labels,
scales and layer classes, without snapshot/visual-regression tooling.

Covers: mat_to_nativeRaster() (pure matrix-to-raster encoding, including
the zero-length edge case), plot_interactive(), cubic_root_trans(),
as.data.frame.GCIMSSample(), plot() for GCIMSSample/GCIMSChromatogram/
GCIMSSpectrum (labels, data, axis ranges, the single/multi/empty-value
subtitle branches), and overlay_peaklist() across its color_by, apex,
palette-recycling and pdata-merging branches, plus its two validation
errors. Brings plot-GCIMSSample.R, plot-GCIMSChromatogram.R,
plot-GCIMSSpectrum.R and utils-plot.R to 100% line coverage.

While writing these tests, found that mat_to_gplot() checked
inherits(trans, "trans") to validate a transform object passed directly
to plot()'s trans= argument (as opposed to a string). scales >= 1.3.0
renamed this S3 class from "trans" to "transform", so passing any
transform object -- including the package's own cubic_root_trans() --
this way always failed with "unknown trans value", even though the
roxygen docs explicitly point to this as valid usage. Fixed the check to
match the class scales has used since 1.3.0 (already over a year old),
and bumped the scales dependency in DESCRIPTION to >= 1.4.0 accordingly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019V3CHRGSzcEu3k6tpUFv56
@zeehio

zeehio commented Jul 12, 2026

Copy link
Copy Markdown
Member Author

On this sprint we've gone from testing 23% of the code lines to 86% of the code lines.

A higher test coverage helps to understand if something is breaking because of a dependency upgrade or because an unintended regression in a recent code change. It also gives confidence when making a change to see that the change we are making doesn't break the code we have so far. It also helps detecting issues in less visited code paths, such as error handling or corner features.

During this process we have fixed a dozen bugs.

One of these bugs prevents spurious peaks from appearing in the peak table, before I remember we were filtering them out manually.

Another bug fixed the saturation column in the peak lists. This column was not used in downstream analysis, but still it is a useful QC metric.

The other 10 fixed bugs would not be triggered usually, but still it's worth having them fixed.

@zeehio
zeehio merged commit bb3e02f into master Jul 12, 2026
1 check passed
@zeehio
zeehio deleted the claude/gcims-plot-coverage branch July 12, 2026 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants