Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding-plotBuilder #345

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

bmatyasb
Copy link

No description provided.

@FBartos FBartos self-requested a review January 30, 2025 08:00
@FBartos
Copy link
Contributor

FBartos commented Jan 30, 2025

The PR above fixes the wrapper issues. You should be able to load the module and get the main analysis function running.
(you now get errors about undefined '%>%' command)

@FBartos
Copy link
Contributor

FBartos commented Jan 30, 2025

The code is nicely formated and quite readable, I think it's pretty much compliant with the JASP style. Very nice! There are two issue I will need you to help us with (which is also what Don mentioned in the Mattermost channel). That's:

  1. not importing the complete package namespaces, i.e., your original NAMESPACE file had,
import(jaspBase)
import(jaspGraphs)
import(ggplot2)
import(tidyplots)
import(rlang)
import(dplyr)
import(tidyr)
import(ggprism)
import(patchwork)
import(tidyverse)
import(ggpubr)
import(ggeasy)
import(forcats)

but this easily messes the internal JASP namespace. Instead, it's highly preferred to use e.g.,
ggplot2::ggplot() + ggplot2::theme_void() notation

  1. trimming down on some of the large package dependencies wherever possible. The main issues are
  • tidyverse
  • tidyplots
  • tidyr
  • dplyr
    (am I missing some @vandenman ?). We cannot really ship those packages because a) they have an extremely large dependency base and the potential of breaking JASP during the update is too big, and b) they are most wrappers meant for analysis scripts but not for package development - they are not very stable and they might change (which would brick the analysis). Maybe Don has a more precise explanation on this.

This will, unfortunately, require some rewriting of the code, but pretty much tidyverse convenience functions have baseR alternatives. I don't really use tidyverse at all so it's a bit challenging to me figure out what are all the imported functions (apart from the pipe signs haha), but I would guess that using ChatGPT like tool could make this not too difficult. Does this seem reasonable to you and that it would be manageable for you to implement?

@FBartos FBartos requested a review from vandenman January 30, 2025 08:29
@bmatyasb
Copy link
Author

bmatyasb commented Jan 30, 2025 via email

@FBartos
Copy link
Contributor

FBartos commented Jan 30, 2025

tidyr and dplyr are both imported by tidyplots (so removing them while keeping tidyplots won't help that much). I think tidyverse has to go for sure, but @vandenman will have the final say for the rest

@EJWagenmakers
Copy link

As an aside, I have asked @PabRod to stress this point in the documentation. If we make a list of things that contributors ought to be aware of, this one might break into the top 3....

@bmatyasb
Copy link
Author

bmatyasb commented Jan 30, 2025 via email

@EJWagenmakers
Copy link

EJWagenmakers commented Jan 30, 2025

Also, will this still be as relevant when we have our online module library in llace? I know this will take a while to get done, but still useful to know [correction: we want this in Des riptives, so part of the JASP core functionality]

@vandenman
Copy link
Contributor

Going by the list here I would suggest

package keep
ggplot2 👍
tidyplots 👍
rlang 👍
dplyr 👍
tidyr 👍
forcats 👍
patchwork 👍
ggprism
tidyverse
ggeasy
ggpubr

ggpubr is a bit of a questionable case, because since both jaspGraphs and tidyplots use it it is reeled in anyway. But in general, try to keep the dependencies as lean as possible. So don't use tidyverse directly but figure out what subpackage you need from it. Likewise, we prefer not to reel in things like ggeasy and instead do the theming manually. Regarding ggprism, it's tricky to figure out where you use it at the moment (due to import instead of importFrom in the namespace) but if it's about the color schemes then we typically add these in jaspGraphs.

A good rule of thumb with dependencies (that we sadly aren't very strict about) is that everything in github.com/tidyverse and github.com/r-lib is fine to use, and the rest we look at on a case-by-case basis.

@vandenman
Copy link
Contributor

also, you need to adjust the DESCRIPTION file otherwise the unit tests will surely fail to install the package.

@bmatyasb
Copy link
Author

bmatyasb commented Jan 30, 2025 via email

@vandenman
Copy link
Contributor

what about ggpubr::geom_bracket? that looks quite similar to ggprism::add_pvalue.

@bmatyasb
Copy link
Author

bmatyasb commented Jan 30, 2025 via email

@bmatyasb
Copy link
Author

bmatyasb commented Jan 30, 2025 via email

@bmatyasb
Copy link
Author

bmatyasb commented Jan 30, 2025 via email

@bmatyasb
Copy link
Author

bmatyasb commented Jan 30, 2025 via email

@FBartos
Copy link
Contributor

FBartos commented Jan 31, 2025

I think library(package) should never be done because it loads the whole package namespace into the current package (which can create a lot of mess).

When JASP loads a module, it loads the namespace of all its analyses (in the same way as if you develop an R package). It is very easy to get namespace conflicts among analyses within the same module. Therefore, the package::function format is almost always preferred as it ascertains that the correct function is called (even when multiple packages export the same function) and the overall module namespace remains concise.

@vandenman
Copy link
Contributor

Am I allowed to use import() or importFrom(), or library() inside the
script, or should I use the package::function format?

never library, import is fine for e.g., jaspBase. importFrom I'd use if you use have a function from a package that you use often. pkg::function I'd use in most other cases.

@bmatyasb
Copy link
Author

bmatyasb commented Feb 3, 2025 via email

@bmatyasb
Copy link
Author

bmatyasb commented Feb 3, 2025

Sorry, the addition of the #2 tag was completely accidental

Copy link
Contributor

@vandenman vandenman left a comment

Choose a reason for hiding this comment

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

Just went over the QML, see the comments below. Overall this looks really good and complete, thanks!

Form {
columns: 1

infoBottom:
Copy link
Contributor

Choose a reason for hiding this comment

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

Two things.

  1. This looks generated. If it is, can you add the R code to generate this as a comment above this for future updates?
  2. You can do string continuation with \ (see https://forum.qt.io/topic/113667/multiline-strings/2). Not sure how to handle \n though.

/// Here begins the main plot control
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////

TabView {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please indent this with tabs instead of spaces? :)

Comment on lines 2360 to 2372
DropDown {
name: "sortYLabelsOrder"
label: "Sort Y labels"
values: ["Increasing", "Decreasing"]
startValue: "Increasing"
}

DropDown {
name: "aggregationFunY"
label: "By"
values: ["mean", "median"]
startValue: "mean"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

qsTr for the labels and see other comment about values and startvalues.

Comment on lines 2554 to 2573
DoubleField {
name: "topMargin"
label: "Top"
value: 10
}
DoubleField {
name: "bottomMargin"
label: "Bottom"
value: 10
}
DoubleField {
name: "leftMargin"
label: "Left"
value: 10
}
DoubleField {
name: "rightMargin"
label: "Right"
value: 10
}
Copy link
Contributor

Choose a reason for hiding this comment

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

labels need qsTr.

Comment on lines 2494 to 2519
{ label: "Default: JASP colors", value: "colors_JASP" },
{ label: "Discrete: Friendly", value: "colors_discrete_friendly" },
{ label: "Discrete: Seaside", value: "colors_discrete_seaside" },
{ label: "Discrete: Apple", value: "colors_discrete_apple" },
{ label: "Discrete: Friendly Long", value: "colors_discrete_friendly_long" },
{ label: "Discrete: Okabe Ito", value: "colors_discrete_okabeito" },
{ label: "Discrete: IBM", value: "colors_discrete_ibm" },
{ label: "Discrete: Metro", value: "colors_discrete_metro" },
{ label: "Discrete: Candy", value: "colors_discrete_candy" },
{ label: "Discrete: Alger", value: "colors_discrete_alger" },
{ label: "Discrete: Rainbow", value: "colors_discrete_rainbow" },
{ label: "Continuous: Viridis", value: "colors_continuous_viridis" },
{ label: "Continuous: Magma", value: "colors_continuous_magma" },
{ label: "Continuous: Inferno", value: "colors_continuous_inferno" },
{ label: "Continuous: Plasma", value: "colors_continuous_plasma" },
{ label: "Continuous: Cividis", value: "colors_continuous_cividis" },
{ label: "Continuous: Rocket", value: "colors_continuous_rocket" },
{ label: "Continuous: Mako", value: "colors_continuous_mako" },
{ label: "Continuous: Turbo", value: "colors_continuous_turbo" },
{ label: "Continuous: Blue Pink Yellow", value: "colors_continuous_bluepinkyellow" },
{ label: "Diverging: Blue to Red", value: "colors_diverging_blue2red" },
{ label: "Diverging: Blue to Brown", value: "colors_diverging_blue2brown" },
{ label: "Diverging: BuRd", value: "colors_diverging_BuRd" },
{ label: "Diverging: BuYlRd", value: "colors_diverging_BuYlRd" },
{ label: "Diverging: Spectral", value: "colors_diverging_spectral" },
{ label: "Diverging: Icefire", value: "colors_diverging_icefire" }
Copy link
Contributor

Choose a reason for hiding this comment

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

labels need qsTr

updatedPlots <- plotResults$updatedPlots

if (!is(jaspResults[["plotGridContainer"]], "JaspContainer")) {
plotGridContainer <- createJaspContainer(title = "Plot Grid")
Copy link
Contributor

Choose a reason for hiding this comment

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

    plotGridContainer <- createJaspContainer(title = gettext("Plot Grid"))

we should mark this titles text by gettext/gettextf to make it translatable.

Comment on lines 48 to 62
name: "PlotBuilderTab"

rowComponent: Group {

childControlsArea.anchors.leftMargin: jaspTheme.contentMargin

Group{
columns:2
TextField {
name: "plotId"
label: qsTr("Plot ID")
fieldWidth: 200
placeholderText: qsTr("Enter Plot ID")
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: "PlotBuilderTab"
rowComponent: Group {
childControlsArea.anchors.leftMargin: jaspTheme.contentMargin
Group{
columns:2
TextField {
name: "plotId"
label: qsTr("Plot ID")
fieldWidth: 200
placeholderText: qsTr("Enter Plot ID")
}
name: "PlotBuilderTab"
newItemName: qsTr("Plot 1")
rowComponent: Group {
childControlsArea.anchors.leftMargin: jaspTheme.contentMargin
Group{
columns:2

Adding the newItemName automatically names the tabs "Plot 1", "Plot 2", ...
image
(and users can rename them by double clicking them)

These can be also used as the plotId - a bit of an annoying issue now is that you need to fill in the idea first, otherwise the whole analysis is terminated with an error. Using the tab names circumvents the issue (in case you names without spaces and special symbols, it should be easy to replace them with underscores using gsub etc?)

This needs to be propagated into the R-code. While now the code takes the plotId from options[["PlotBuilderTab"]][[1]][["plotId"]], now it should refer to the name in options[["PlotBuilderTab"]][[1]][["value"]]
(I did not do that as it's a too big change for a suggested change here)

Copy link
Author

Choose a reason for hiding this comment

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

that's a great idea! thanks a lot! just a couple of lines to rewrite and it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks :) I will do a deeper dive and more suggestions once all the previous parts are addressed (it's a bit of a mess with al the qml suggestions right now) Could you maybe just prompt me to do so on mattermost?

bmatyasb and others added 5 commits February 4, 2025 13:26
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.

5 participants