Skip to content

type_abline()-alike now respect flip = TRUE - #735

Merged
grantmcdermott merged 7 commits into
mainfrom
hvline_flip
Sep 16, 2026
Merged

grantmcdermott merged 7 commits into
mainfrom
hvline_flip

Conversation

@grantmcdermott

Copy link
Copy Markdown
Owner

Fixes #733

I ended up consolidating much of the type_ab/h/v/line() code in the process. I also had to update some ancillary examples that were using the (wrong) workaround for adding straight lines to flipped plots. This may end up triggering revdep issues for other packages, so we'll need to check carefully.

MWE

Using the same motivating examples as #733...

pkgload::load_all("~/Documents/Projects/tinyplot")
#> ℹ Loading tinyplot
dat = data.frame(x = -1:1, y = 0:2)
plt(y ~ x, data = dat)
plt_add(type = "vline", lty = 2, flip = TRUE)

coefplot = function(mod, ...) {
  coefs = data.frame(names(coef(mod)), coef(mod), confint(mod))
  colnames(coefs) = c("term", "est", "lwr", "upr")
  plt(
    est ~ term, ymin = lwr, ymax = upr, data = coefs,
    type = "errorbar",
    theme = "classic",
    ...
  )
  plt_add(type = "hline", lty  = 4, col = "hotpink")
}
mod = lm(mpg ~ 0 + wt / factor(am), mtcars)
coefplot(mod, flip =TRUE)

Created on 2026-09-16 with reprex v2.1.1

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved moderate validation and mixed-slope transformation issues remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR makes type_abline(), type_hline(), and type_vline() respect flipped plots through shared rendering logic.

Changes:

  • Adds flip-aware line orientation and slope transformations.
  • Consolidates straight-line implementations.
  • Updates examples, documentation, NEWS, and snapshot coverage.

Review findings:

  • R/type_abline.Rmoderate (1 vote): NULL parameters can bypass validation; mixed zero/nonzero slopes are transformed incorrectly.
  • R/type_abline.Rnit (1 vote): Add flipped zero-slope and grouped/faceted coverage.
  • R/type_abline.Rnit (1 vote): Add active unflipped type_abline() coverage, including b = 0.
  • vignettes/gallery_figs/likert.Rnit (2 votes): Update the remaining flipped Likert examples in R/type_barplot.R and generated documentation.
File summaries
File Summary
vignettes/gallery_figs/likert.R Updates flipped Likert examples.
R/type_vline.R Delegates vertical-line rendering to shared logic.
R/type_hline.R Delegates horizontal-line rendering to shared logic.
R/type_errorbar.R Updates a flipped example.
R/type_abline.R Consolidates line rendering and flip transformations.
NEWS.md Documents the behavior change.
man/type_errorbar.Rd Updates generated errorbar documentation.
man/type_abline.Rd Updates generated line documentation.
inst/tinytest/test-type_pointrange.R Updates a flipped layer example.
inst/tinytest/test-flip.R Adds flipped-line snapshot coverage.
inst/tinytest/_tinysnapshot/flip_ablines.svg Stores expected snapshot output.
Review details

Files not reviewed (2)

  • man/type_abline.Rd: Generated file
  • man/type_errorbar.Rd: Generated file

Suppressed comments (4)

R/type_abline.R:117

  • Filtering NULL before validation drops the active argument for calls such as type_hline(h = NULL) and type_vline(v = NULL). These constructors previously called assert_numeric() on their supplied parameter, so this is a validation regression that can defer the failure to an empty abline() call. Build params from the selected line type and validate those entries before removing unused fields.
  params = params[!vapply(params, is.null, logical(1))]

R/type_abline.R:188

  • With a numeric continuous by, the parameter vectors remain unsliced because the !by_continuous branches are skipped, so b can contain both zero and nonzero slopes. The all() check only handles the all-zero case; mixed vectors turn the zero-slope entries into a = -a / 0 and b = 1 / 0 rather than drawing those entries as vertical v = a lines. Split zero-slope entries and transform only the nonzero slopes, preserving the matching aesthetics for each line.
        abline = if (all(params[["b"]] == 0)) {
          list(v = params[["a"]])
        } else {
          list(a = -params[["a"]] / params[["b"]], b = 1 / params[["b"]])

R/type_abline.R:185

  • The new flipped snapshot covers scalar h/v values and a nonzero-slope abline, but not this b == 0 branch. That branch is the special-case transformation for a horizontal original line and can regress independently; please add a flipped zero-slope abline test (and, ideally, a grouped/faceted parameter-vector case) so the new shared implementation's edge paths are protected.
        abline = if (all(params[["b"]] == 0)) {

R/type_abline.R:193

  • The active tests still do not render an unflipped type_abline(): inst/tinytest/test-type_abline.R only checks its no-data error, while the new snapshot exercises only the flipped path. Since this refactor replaces the entire a/b drawing implementation, add an active unflipped case (including the b = 0 conversion branch) to catch regressions in the common path.
    do.call(abline, c(params, list(col = icol, lty = ilty, lwd = ilwd)))
  • Files reviewed: 8/11 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread vignettes/gallery_figs/likert.R
@grantmcdermott
grantmcdermott merged commit 073c951 into main Sep 16, 2026
3 checks passed
@grantmcdermott
grantmcdermott deleted the hvline_flip branch September 16, 2026 18:58
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.

type "v/hline" don't respect flip = TRUE

2 participants