Fix ignored position argument in violin jitter layer - #106
Merged
Merged
Conversation
Co-authored-by: max.karlsson <max.karlsson@pixelgen.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The core fix and associated test/import/changelog updates are coherent and low-risk, addressing the warning and restoring intended dodge behavior.
Pull request overview
This PR fixes an incorrect use of position with ggbeeswarm::geom_quasirandom() in plot_violin(), eliminating a ggplot2 warning and ensuring jittered points are properly dodged with violins.
Changes:
- Update
plot_violin()to passdodge.width = 0.9directly togeom_quasirandom()(instead of supplying apositionobject that gets ignored). - Remove the unused
position_quasirandomimport from package roxygen andNAMESPACE. - Strengthen the existing
plot_violintest to assert no warning is emitted and that the jitter layer usesPositionQuasirandomwithdodge.width = 0.9.
File summaries
| File | Description |
|---|---|
| tests/testthat/test_plot_helpers.R | Updates plot_violin test to assert no warning and verify jitter dodging via the layer position object. |
| R/plot_helpers.R | Fixes quasi-random jitter configuration by passing dodge.width directly to geom_quasirandom(). |
| R/pixelatorES-package.R | Removes the now-unused position_quasirandom import from roxygen imports. |
| NAMESPACE | Drops position_quasirandom import to match updated usage. |
| CHANGELOG.md | Documents the warning removal and correct dodging behavior for violin plot points. |
Review details
- Files reviewed: 5/5 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 on lines
+791
to
+794
| # geom_quasirandom() builds its own position object from these | ||
| # arguments; a position_quasirandom() object passed as `position` is | ||
| # silently dropped | ||
| dodge.width = 0.9 |
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.
Problem
Rendering a report emitted two warnings per plot pair from
component_abundance_per_marker():The warning came from the jitter layer in
plot_violin():ggbeeswarm::geom_quasirandom()does not take apositionargument. It constructs its ownposition_quasirandom()object from its named arguments (method,width,varwidth,bandwidth,nbins,dodge.width,orientation) and forwards everything else to the layer'sparams. Thepositionobject therefore ended up as an unknown geom parameter:ggplot2warned about it and dropped it, so the intendeddodge.width = 0.9never had any effect. Thewidth = "units"value was also invalid (widthis numeric, the maximum amount of spread) and would have errored if the position object had actually been used.Fix
Pass
dodge.width = 0.9directly togeom_quasirandom()and let it build the position.widthis left at its default, which is derived from the resolution of the (already dodged) x positions, so the point spread adapts to the violin widths instead of being hard-coded.The now-unused
ggbeeswarm::position_quasirandomimport was removed.Verification
Reproduced the warning and confirmed the fix with
ggplot24.0.3 andggbeeswarm0.7.3, running the pre-fix and post-fixplot_violin()bodies side by side against sample data.For the plots the report actually draws today, where each
sample_aliasmaps to a singlecondition, the points are unchanged and only the warning disappears:Abundance violin plot before and after the fix
When a fill variable has several levels per x category, the dropped
dodge.widthwas also visible: the points were spread across the whole category instead of following their own violin, leaving many of them outside any violin. They are now dodged along with the violins.Violin plot with two fill levels per x category, before and after the fix
The built layer data agrees with the images: in the two-level case the point clouds spanned 0.62–1.37 for both node types before the fix, and now sit at 0.604–0.942 and 1.066–1.388, matching their violin extents of 0.55–1.00 and 1.00–1.45.
The existing
plot_violintest now asserts that no warning is raised and that the jitter layer is dodged by 0.9.To show artifacts inline, enable in settings.