Migrate plotting argument routing from do.call to ParmOff#15
Draft
Copilot wants to merge 3 commits into
Draft
Conversation
Agent-Logs-Url: https://github.com/asgr/magicaxis/sessions/9135fda2-a646-4b5f-b646-8c7fdec1bca2 Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>
Agent-Logs-Url: https://github.com/asgr/magicaxis/sessions/9135fda2-a646-4b5f-b646-8c7fdec1bca2 Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>
Agent-Logs-Url: https://github.com/asgr/magicaxis/sessions/9135fda2-a646-4b5f-b646-8c7fdec1bca2 Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the plotting helpers in magicaxis so that internal forwarding of ... is done through new ParmOff-backed helpers (.mag_split_args, .mag_pick_args, .mag_defaults, .mag_call) rather than ad-hoc do.call/%in% filtering. It introduces prefixed routing (e.g. magaxis.*, magmap.*, magbar.*, points.*, image.*, magcon.*, magplot.*) so callers can target nested sub-functions explicitly, and adds a testthat suite + a vignette exercising the new behaviour.
Changes:
- Adds
R/ParmOff.Rwith the new split/merge/call helpers, and convertsmagaxis,magplot,plot.magbin,magimage,magimageRGB,magtrito use them. - Adds
testthatinfra (tests/testthat.R, edition 3, focused routing tests) and avignettes/magic axis.Rmdcovering scatter, hist, contour, binning, image, RGB and triangle plots. - Updates
DESCRIPTION(ImportsParmOff,Remotes: asgr/ParmOff, vignette + testthat metadata) and the Rd pages to document the new prefixed targeting.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| R/ParmOff.R | New internal helpers wrapping ParmOff::ParmOff for argument splitting/merging/calling. |
| R/magaxis.R | Routes ... to axis() and mtext() via .mag_call, with new axis./mtext. prefix support. |
| R/magplot.R | Switches the maghist, z-axis magmap/magbar/points dispatch to .mag_call; adds prefix routing. |
| R/magbin.R | Adds magmap./magbar./magplot./points. routing in plot.magbin; converts do.call sites. |
| R/magimage.R, R/magimageRGB.R | Adds image. prefix support; switches do.call('image', ...) to .mag_call. |
| R/magtri.R | Splits axis/contour/points dots so magaxis., magcon., points. can be targeted. |
| tests/testthat.R, tests/testthat/test-parmoff-routing.R | New testthat infra and tests covering the routing helpers. |
| vignettes/magic axis.Rmd | New vignette exercising axes, scatter, hist, contour, binning, image, RGB, triangle. |
| DESCRIPTION | Adds ParmOff Import + Remotes, testthat/vignette metadata. |
| man/*.Rd | Documents the new prefix-based argument targeting. |
Comments suppressed due to low confidence (1)
R/magaxis.R:22
- In the
length(dots) > 0branch, whendotsmtext_base$argsis empty (nocex.lab/col.lab/font.labpassed) butdotsmtext_pref$argsis non-empty (user passed e.g.mtext.cex),dotsmtextstarts as an empty list. The linenames(dotsmtext) = c('cex','col','font')[match(names(dotsmtext), dotskeepmtext)]is a no-op there, which is fine, but the subsequent.mag_defaults(dotsmtext, dotsmtext_pref$args)returns the prefixed args. So far so good — however, whenlength(dots) == 0the else-branch setsdotsmtext = {}(i.e.NULL), so any prefixed values are simply ignored (no prefixed args can exist whenlength(dots) == 0, so that's consistent). Nit: replace{}withlist()for clarity and to keep.mag_call(..., .dots = dotsmtext, ...)working with an explicitly-typed value.
if(length(dots)>0){
dotsaxis_base = .mag_split_args(dots, use_args = dotskeepaxis)
dotsaxis_pref = .mag_split_args(dotsaxis_base$rest, use_args = dotskeepaxis, prefix = 'axis')
dotsaxis = .mag_defaults(dotsaxis_base$args, dotsaxis_pref$args)
dotsmtext_base = .mag_split_args(dotsaxis_pref$rest, use_args = dotskeepmtext)
dotsmtext_pref = .mag_split_args(dotsmtext_base$rest, prefix = 'mtext')
dotsmtext = dotsmtext_base$args
names(dotsmtext)=c('cex', 'col', 'font')[match(names(dotsmtext), dotskeepmtext)]
dotsmtext = .mag_defaults(dotsmtext, dotsmtext_pref$args)
}else{
dotsaxis={}
dotsmtext={}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1
to
+8
| --- | ||
| title: "magicaxis" | ||
| output: rmarkdown::html_vignette | ||
| vignette: > | ||
| %\VignetteIndexEntry{magicaxis} | ||
| %\VignetteEngine{knitr::rmarkdown} | ||
| %\VignetteEncoding{UTF-8} | ||
| --- |
Comment on lines
+12
to
+16
| Suggests: imager, fst, plotly, abind, testthat (>= 3.0.0), knitr, rmarkdown | ||
| Imports: grDevices, graphics, stats, celestial (>= 1.4.1), MASS, plotrix, sm, mapproj, RANN, ParmOff | ||
| VignetteBuilder: knitr | ||
| Config/testthat/edition: 3 | ||
| Remotes: asgr/ParmOff |
Comment on lines
+22
to
+37
| .mag_split_args = function(dots, use_args = NULL, prefix = NULL){ | ||
| if(length(dots) == 0){ | ||
| return(list(args = list(), rest = list())) | ||
| } | ||
| if(is.null(names(dots))){ | ||
| names(dots) = rep('', length(dots)) | ||
| } | ||
| if(is.null(prefix)){ | ||
| keep = names(dots) %in% use_args | ||
| args = .mag_pick_args(dots[keep], use_args = use_args)$current_args | ||
| }else{ | ||
| keep = grepl(paste0('^', prefix, '\\.'), names(dots)) | ||
| args = .mag_pick_args(dots[keep], use_args = use_args, prefix = prefix)$current_args | ||
| } | ||
| list(args = args, rest = dots[!keep]) | ||
| } |
| dotsaxis_split = .mag_split_args(dots, prefix = 'magaxis') | ||
| dotscon_split = .mag_split_args(dotsaxis_split$rest, prefix = 'magcon') | ||
| dotspoints_split = .mag_split_args(dotscon_split$rest, prefix = 'points') | ||
| dotsaxis = dotsaxis_split$args |
Comment on lines
85
to
+105
| @@ -85,23 +92,23 @@ magtri=function(chains, samples=1000, thin=1, samptype='end', grid=FALSE, do.tic | |||
| } | |||
| if(j==1){ | |||
| if(is.null(lab)){ | |||
| magaxis(1,xlab=chaincolnames[i]) | |||
| .mag_call(magaxis, .dots = dotsaxis, side=1, xlab=chaincolnames[i]) | |||
| }else{ | |||
| magaxis(1,xlab=lab[[i]]) | |||
| .mag_call(magaxis, .dots = dotsaxis, side=1, xlab=lab[[i]]) | |||
| } | |||
| } | |||
| }else{ | |||
| plot.new() | |||
| plot.window(xlim=xrange,ylim=yrange) | |||
| magaxis(1:2,grid=grid, grid.col = 'lightgrey',labels=FALSE,do.tick=do.tick) | |||
| points(chains[usesamps,c(i,j)],pch='.',col='darkgrey') | |||
| points(meanvec[i],meanvec[j],col='red',pch=4,cex=2) | |||
| .mag_call(magaxis, .dots = dotsaxis, side=1:2, grid=grid, grid.col='lightgrey', labels=FALSE, do.tick=do.tick) | |||
| .mag_call(points, .dots = .mag_defaults(list(pch='.', col='darkgrey'), dotspoints), x = chains[usesamps,i], y = chains[usesamps,j]) | |||
| .mag_call(points, .dots = .mag_defaults(list(col='red', pch=4, cex=2), dotspoints), x = meanvec[i], y = meanvec[j]) | |||
| split = magicaxis:::.mag_split_args(dots, prefix = "magaxis") | ||
| expect_equal(split$args$cex.axis, 2) | ||
| expect_equal(split$args$cex.lab, 1.5) | ||
| expect_equal(names(split$rest), c("magcon.lty", "pch")) |
| } | ||
| \item{\dots}{ | ||
| Other arguments to be passed to base \code{\link{axis}} and \code{\link{mtext}} functions as relevant. Options for axis are as described in \code{\link{axis}}, but note \option{cex.lab}, \option{col.lab} and \option{font.lab} are parsed as \option{cex}, \option{col} and \option{font} into \code{\link{mtext}} as required. | ||
| Other arguments to be passed to base \code{\link{axis}} and \code{\link{mtext}} functions as relevant. Options for axis are as described in \code{\link{axis}}, but note \option{cex.lab}, \option{col.lab} and \option{font.lab} are parsed as \option{cex}, \option{col} and \option{font} into \code{\link{mtext}} as required. The same arguments can also be supplied explicitly with \option{axis.} and \option{mtext.} prefixes (for example \option{axis.cex.axis} or \option{mtext.cex}). |
| } | ||
| \item{\dots}{ | ||
| Arguments to be parsed to \code{image} and \code{magaxis} as relevant (this is checked for internally by argument name). See \code{\link{image}} and \code{\link{magaxis}} for details. | ||
| Arguments to be parsed to \code{image} and \code{magaxis} as relevant (this is checked for internally by argument name). The same image-specific arguments can also be supplied with an \option{image.} prefix. See \code{\link{image}} and \code{\link{magaxis}} for details. |
Comment on lines
+17
to
+68
| local_mocked_bindings( | ||
| magmap = function(...) { | ||
| args = list(...) | ||
| calls$magmap = c(calls$magmap, list(args)) | ||
| list(map = seq_along(args$data), datalim = range(args$data)) | ||
| }, | ||
| magbar = function(...) { | ||
| calls$magbar = list(...) | ||
| invisible(NULL) | ||
| }, | ||
| points = function(...) { | ||
| calls$points = list(...) | ||
| invisible(NULL) | ||
| }, | ||
| plot = function(...) invisible(NULL), | ||
| magaxis = function(...) invisible(NULL), | ||
| .package = "magicaxis" | ||
| ) | ||
| magplot(1:3, 4:6, z = 7:9, magmap.locut = 0.25, position = "bottomleft", points.pch = 3, cex = 2) | ||
| expect_equal(calls$magmap[[1]]$locut, 0.25) | ||
| expect_equal(calls$magbar$position, "bottomleft") | ||
| expect_equal(calls$points$pch, 3) | ||
| expect_equal(calls$points$cex, 2) | ||
| }) | ||
|
|
||
| test_that("magtri routes prefixed arguments to magaxis, magcon and points", { | ||
| calls = new.env(parent = emptyenv()) | ||
| calls$magaxis = list() | ||
| calls$magcon = list() | ||
| calls$points = list() | ||
| local_mocked_bindings( | ||
| magaxis = function(...) { | ||
| calls$magaxis = c(calls$magaxis, list(list(...))) | ||
| invisible(NULL) | ||
| }, | ||
| magcon = function(...) { | ||
| calls$magcon = c(calls$magcon, list(list(...))) | ||
| invisible(NULL) | ||
| }, | ||
| points = function(...) { | ||
| calls$points = c(calls$points, list(list(...))) | ||
| invisible(NULL) | ||
| }, | ||
| plot = function(...) invisible(NULL), | ||
| density = function(...) structure(list(x = 1:2, y = c(1, 1)), class = "density"), | ||
| layout = function(...) invisible(NULL), | ||
| par = function(...) invisible(NULL), | ||
| plot.window = function(...) invisible(NULL), | ||
| plot.new = function(...) invisible(NULL), | ||
| box = function(...) invisible(NULL), | ||
| abline = function(...) invisible(NULL), | ||
| .package = "magicaxis" |
| } | ||
| \item{\dots}{ | ||
| Further arguments to be passed to: base \code{\link{plot}}; \code{\link{magaxis}} -> \code{\link{axis}}; \code{\link{magmap}} and \code{\link{magbar}} if \option{z} scaling is being used. | ||
| Further arguments to be passed to: base \code{\link{plot}}; \code{\link{magaxis}} -> \code{\link{axis}}; \code{\link{magmap}} and \code{\link{magbar}} if \option{z} scaling is being used. When plotting with a \option{z} scale, arguments can be targeted explicitly with \option{magmap.}, \option{magbar.} and \option{points.} prefixes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This updates plotting helpers to use
ParmOffinstead of ad hocdo.calldispatch, so nested plotting arguments can be targeted explicitly and forwarded consistently. It also adds coverage for the new routing behavior and a vignette that exercises a broader slice of plotting functionality.Argument routing
do.call-based forwarding with sharedParmOffhelpers in the plotting stack.magaxis.*,mtext.*magcon.*,points.*magmap.*,magbar.*,magplot.*image.*magaxis,magplot,plot.magbin,magimage,magimageRGB, andmagtri.Higher-level plotting behavior
magtri()can now pass arguments independently to axis drawing, contour generation, and point layers instead of pushing everything throughmagcon(...).magplot()/plot.magbin()now split shared...cleanly between plot setup, color mapping, color bars, and point rendering.Package/docs updates
ParmOffas a package dependency.Tests and vignette
magplot()andmagtri().vignettes/magic axis.Rmdwith broader examples covering axes, scatter plots, histograms, contours, binning, images, RGB images, and triangle plots.Example of the new routing style: