Skip to content

Plot builder fixes - #480

Open
matyasbukva wants to merge 6 commits into
jasp-stats:masterfrom
matyasbukva:plotBuilderFixes
Open

Plot builder fixes#480
matyasbukva wants to merge 6 commits into
jasp-stats:masterfrom
matyasbukva:plotBuilderFixes

Conversation

@matyasbukva

Copy link
Copy Markdown
Contributor

@matyasbukva

Copy link
Copy Markdown
Contributor Author

@vandenman I pushed this two months ago, but I'm only now seeing all these failed tests. What's causing this? Also, could someone take a look at it to see if it can be fixed using the plot builder?

@matyasbukva

Copy link
Copy Markdown
Contributor Author

I deleted the last commit because the issue has already been resolved in version 0.97

@boutinb

boutinb commented May 20, 2026

Copy link
Copy Markdown
Contributor

Hmm the unit tests are still failing. No clue why... @vandenman ?

@matyasbukva

Copy link
Copy Markdown
Contributor Author

I'd like to remove the "beta" label from the plot builder eventually, but unfortunately, my current pull request always gets left behind in the new releases, so things aren't moving forward. Could you take a look at it?

@EJWagenmakers

Copy link
Copy Markdown

Yes, let's remove that label please!

@tomtomme

Copy link
Copy Markdown
Member

@boutinb @vandenman
no solution yet for the failing unit tests?

@tomtomme
tomtomme requested a review from vandenman June 11, 2026 17:35
@tomtomme

Copy link
Copy Markdown
Member

Are these failing unit tests being worked on? If we want to get rid of the beta-label it would be really important to get this in, would it not?

@matyasbukva

Copy link
Copy Markdown
Contributor Author

Is there any progress here? There are already a lot of issues with the plot builder, and I'd like to update it, too.

@tomtomme

Copy link
Copy Markdown
Member

@matyasbukva
I also do wonder. Did you try to reach out via the developer chat at mattermost? Maybe it is easier to get answers there.

@tomtomme

Copy link
Copy Markdown
Member

@boutinb : can we have a review on this one please or an explanation on the failing unit tests or what can be done?

better axis and margin  and p value handling logic.
better axis and margin  and p value handling logic.
…dding

Rewriting the Y axis block lost two options along the way. The Y axis
title (titleYPlotBuilder) and the "cut short scale" checkbox
(cutShortScaleY) were no longer passed to tidyplots::adjust_y_axis, so
both fields sat in the interface doing nothing while their X axis
counterparts kept working.

The padding then went the other way and became too strict: an option that
is not in the options at all reads back as numeric(0), and is.finite() on
that gives logical(0), which makes `||` stop with "missing value where
TRUE/FALSE needed". So a plot whose options predate these fields errored
out instead of falling back. Fall back per value on anything that is not a
single finite number, to the 0.1 the qml offers rather than the 0.05 that
was written here, and give the X axis the same treatment since it passed
the raw option straight through.

Finally, translate the comments to English.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@boutinb

boutinb commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

I'm not a R expert, and @vandenman is not maintaining anymore the module. I have asked Claude, below is what I get, and I asked him (it?) to fix the main issues (in the last commit).

The core idea is sound, but the rewrite dropped three Y-axis options, and two of them are now dead.

Blocking

  1. The Y-axis title no longer works. The rewrite deleted:

titleValueY <- tab[["titleYPlotBuilder"]]
if (!is.null(titleValueY) && titleValueY != "") adjust_args_yaxis$title <- titleValueY

and nothing replaces it — there is no title anywhere in the tidyplots block (~L1690–1900) except the X one at L1702. Meanwhile titleYPlotBuilder is still a live QML field (inst/qml/jaspPlotBuilder.qml:2299), so a user typing a Y-axis title now sees nothing happen, while the X title still works. Looks like collateral damage from restructuring the block rather than intent.

  1. cutShortScaleY is now a dead control. adjust_args_yaxis$cut_short_scale <- isTRUE(tab[["cutShortScaleY"]]) was removed, and grep finds zero references to cutShortScaleY in the R source afterwards. The checkbox at jaspPlotBuilder.qml:2330 does nothing. X's equivalent (cutShortScale, L1741) is untouched, so the two axes are now asymmetric.

  2. Non-English comments. The new comments are Hungarian — # --- Limit beállítások ---, # --- Breaks beállítása ---, # --- Padding beállítása ---, # --- Tengely beállítása ---, # --- Opcionális Y-tengelyrendezés ---. These need translating before merge.

Worth checking

  1. Removing the automatic Y-limit expansion may clip p-value brackets. The deleted block computed required_y_max from yPositionPValue + stepDistance × (n−1) and added (n−1) × 0.15 of top padding, specifically so pairwise-comparison annotations fit. Removing it is plausibly the actual fix for "size commands are ignored" (it overrode user limits), but nothing now guarantees the brackets stay inside the panel. The QML default padding bump (0.05 → 0.1) only partly compensates and doesn't scale with the number of comparisons. Worth re-testing with the reporter's Body Weight_NEW.zip with several pairwise comparisons enabled, not just the size complaint.

  2. Padding defaults disagree, and X/Y are handled differently. The R fallback is 0.05:

ifelse(is.finite(bottom_padding), bottom_padding, 0.05)

but the QML default is now 0.1. Also, the new is.finite guarding applies only to Y — X still does adjust_args_xaxis$padding <- c(XPaddingFirst, XPaddingSecond) raw (L1746–1748), so an empty X padding field passes NA straight to tidyplots. Either give X the same treatment or make the fallback 0.1 to match the QML.

  1. Minor R hazard. nchar(trimws(tab[["titlePlotBuilder"]])) > 0 inside &&: the preceding guard is length(...) > 0, so a length > 1 value reaches &&, which is an error in R ≥ 4.3. nzchar() on the first element, or wrapping in isTRUE(), is safer.

  2. Noise. Three blank lines added before the Y block plus a doubled blank line later; easy to drop.

What's good

The empty-title guard (nchar(trimws(...)) > 0) is a real fix — previously a whitespace-only title added an empty title element. The indentation corrections around the isRM block are fine. And restructuring the Y block into limits → breaks → padding is genuinely more readable than what it replaced.

One thing I checked that is not a problem: removing adjust_args_yaxis$rotate_labels does not break rotateYLabel — it's still applied via the ggplot2 theme at L2160. If anything that removes a double rotation for Y; note X is still rotated twice (L1740 and L2156), which may be worth a follow-up.

@boutinb

boutinb commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

The unit tests are still failing. Copilot tells that it's probably due to dependency update (particularly ggplot2 or scales package). I know that there is are a lot of things going on with ggplot2, so I think it's better that someone knowing more about it tries to solve this problem. @FBartos maybe?

@boutinb
boutinb requested a review from FBartos August 31, 2026 14:49
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.

[Bug]: Problem with plot size under Plot Builder (beta)

5 participants