Repository navigation
Conversation
All three contained the identical no-op line:
if (is.numeric(col) & col < length(vars)) col <- col
so a numeric `col` was never interpreted at all. It fell through to `fill <- col`
and reached ggpubr as a bare number, which ggplot2 reads as a base R colour
code - qboxplot(tg, col = 2) produced #DF536B. That matched neither the
documented behaviour ("the index or name of the column") nor qbarplot() and
qhistogram(), where a number is a palette index.
PR #87 previously proposed reading it as a *column* index; that was closed. Per
the design discussion, a number is now a *palette* index everywhere, which is
also what the Q1 answer specified.
Adds one internal resolver, .resolveColToFill(), used by all three functions so
the semantics cannot drift apart again:
1. a character matching a column name -> returned unchanged, so ggpubr groups
by that column and colours it through palette_use (unchanged behaviour)
2. a single positive whole number, or a digit-only string such as "2" ->
looked up in palette_use
3. anything else -> returned unchanged and used as a literal colour
(unchanged behaviour)
The index detection reuses the hardened predicate from the edit to #97:
length 1, non-NA, >= 1, and whole-number numeric or digit-only string. The
palette is sized to cover the requested index so pal[i] cannot yield NA.
qboxplot needed one extra detail: by the time `col` is resolved, its
`palette_use` has already been overwritten with the recycled `fill` colour, so
the resolver is given `palette_use_bac` (the palette as requested) instead.
Without this, every index resolved to the fill colour ("gold").
Behaviour change, deliberate and documented: col = <number> now yields a
palette colour rather than a base R colour code. col = NULL, column names, and
literal colours are all unchanged.
Verified for all three functions across col = NULL, 1, 2, 5, "2", a column
name, and a literal colour, plus a custom palette vector and list input. All
three now return identical colours for identical input.
…)[1] default
No behaviour change - documentation and clarity only.
The default was written as `col = c(NULL, 3)[1]`, which reads as "NULL or 3,
take the first" but always evaluates to 3, because c() silently drops NULL.
Compare qqSave's `plot_on_page = list(FALSE, "A4p", ...)[[1]]`, where the
list() form does correctly preserve the alternatives. Replaced with a plain
`col = 3` plus a comment recording what the old idiom actually did, so nobody
re-derives NULL as a supported option.
The roxygen said only "Color of the plot", which hid three things that cost me
time during the color audit:
- `col` is passed straight to ggpubr::ggscatter(color = ), with no
interpretation by ggExpress at all;
- a number here is therefore a base R colour code (the default 3 is green),
NOT an index into palette_use as in qbarplot() and qhistogram();
- colouring by a *numeric* column fails outright ("Continuous value supplied
to a discrete scale"), because palette_use is a discrete scale; only
discrete columns work.
Deliberately NOT changed: the numeric semantics. Making a number a palette
index here would be consistent with the rest of the package, but 3 is also the
default, so it would change the point colour of every existing qscatter call
from green to palette colour 3. That needs a decision rather than a silent
change.
Verified unchanged: default -> colour 3, col = 2 -> colour 2, col = "red" ->
red.
q32vA4_grid_plot() and qA4_grid_plot() defaulted h/w to hA4*scale and wA4*scale, but hA4/wA4 were never defined anywhere in the package, erroring on any default call. Added them as real parameters (11.69 x 8.27 inches, matching the A4 dimensions used elsewhere in this file). Also fixed the h/w roxygen descriptions, which had hA4/wA4 swapped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per feedback: hA4/wA4 are meant to be optional background variables a
caller can predefine (e.g. in their session) to change plot defaults
package-wide. Adding them as new function parameters broke positional
argument order and didn't support that workflow. Use
get0("hA4", ifnotfound = 11.69) / get0("wA4", ifnotfound = 8.27)
directly in the h/w defaults instead, in q32vA4_grid_plot() and
qA4_grid_plot(). Regenerate the corresponding .Rd files and bump the
package version per AGENTS.md.
Co-authored-by: Abel Vertesy <5101911+vertesy@users.noreply.github.com>
Fix undefined hA4/wA4 constants in q32vA4_grid_plot()/qA4_grid_plot()
Document qscatter's col semantics and remove the misleading c(NULL, 3)[1] default
Make numeric col a palette index in qboxplot, qviolin and qstripchart
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3467c91a98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| palette_use | ||
| } | ||
| return(pal[col_i]) |
There was a problem hiding this comment.
Problem: .resolveColToFill() returns NA when col exceeds the length of a custom palette_use, so the plot can lose its fill or fail inside ggpubr. Trigger: This happens with calls such as qviolin(data, col = 3, palette_use = c("red", "blue")), even though the documentation accepts any positive palette index. Fix: Add the required compact input assertion and either reject out-of-range indexes or define how custom palettes should be extended.
AGENTS.md reference: AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
|
could you please address the codex comment: "Validate custom palette indexes before returning a color Problem: .resolveColToFill() returns NA when col exceeds the length of a custom palette_use, so the plot can lose its fill or fail inside ggpubr. Trigger: This happens with calls such as qviolin(data, col = 3, palette_use = c("red", "blue")), even though the documentation accepts any positive palette index. Fix: Add the required compact input assertion and either reject out-of-range indexes or define how custom palettes should be extended." |
|
Claude finished @vertesy's task in 1m 53s —— View job Addressed the Codex review comment
Fix (
Note: R is not available in this sandbox, so I could not run Pushed as commit |
Previously a col index beyond the length of a custom palette_use vector
(e.g. qviolin(data, col = 3, palette_use = c("red","blue"))) silently
resolved to NA. Named palettes are still auto-regenerated to cover the
requested index, but custom vectors cannot be extended, so an
out-of-range index now throws a clear error instead.
Co-authored-by: Abel Vertesy <5101911+vertesy@users.noreply.github.com>
No description provided.