-
-
Notifications
You must be signed in to change notification settings - Fork 212
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
Bayesian Anova updates #3071
Bayesian Anova updates #3071
Conversation
Travis fails because of the network analysis.. 😕 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frans just updated the R framework; I'm guessing bootnet was updated and their output changed.
Before I delve deeper into your R code, can you fix the tabs/spaces? There are a lot of misaligned lines which makes it kinda hard to read. (I'm guessing the original files had different spacing and you copied some stuff?) |
Bootnet did change recently and some of the computations were fixed, which is why the output changed. https://github.com/SachaEpskamp/bootnet/blob/master/NEWS |
Great work on the code by the way. |
@TimKDJ sorry, the code should be more legible now! |
e75541f
to
70ffd74
Compare
fbb38be
to
7095248
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so you made lots of changes and understandably that introduces some bugs. I added code specific comments and also noted the following problems when testing:
- If you change the Bayes Factor type then the entire table is removed and a new progress bar appears aswell (but also when you tick “Descriptives”, or “effects”…)
- It feels strange that the progress bar runs twice when you tick “estimates”, people can’t track overall progress now
- Descriptives table has some errors:
- Model averaged posteriors plot does not handle interactions very well:
- When you change options not related to the Analysis of Effects table (Compare to best model / % credible interval) then the table is not reused
- If you add some fixed factors and then afterwards tick “Individual plot per level” the progress bar goes haywire
- Colors of Model averaged posteriors don’t feel very jaspy. They’re ggplot defaults?
- Post Hoc Test note is repeated “Note. not enough observations” also not clear to what row it points:
- Marginal posterior plot for RM ANOVA generates a plot per row in the dataset
- custom y-axis label of the descriptives plot in RM ANOVA does not work
- We have the note “Note. All models include Region” do we then also need “Null model (incl. Region)” in the table? (also in 0.9.2.)
- Bayesian ANCOVA fails (“Error in seq.default(2, lInd, 2): wrong sign in 'by' argument”) when one covariate and one fixed factor are added and “Model averaged posteriors” is ticked:
- Should we also take Log(posterior odds) in post hoc tests when LogBF is ticked? (also in 0.9.2.)
- If you try to create plots in “Single Model Inference” you cannot have a covariate in the “Specific model terms” otherwise only empty plots are generated (even when you do have a fixed factor)
ys <- ys + weights[i] * density(samples[[i]], from = fromTo[1L], to = fromTo[2L], n = n)$y | ||
} | ||
|
||
return(list(x = xs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe set an upper bound and change the calculation if the size is too big?
7095248
to
a935d65
Compare
@TimKDJ I think this is ready for another review. Three things I wasn't able to fix:
For the post hoc, there is only one footnote but it's still not shown in the table where it points to. Oh I also still need to implement the memory-safe but slow alternative. |
52b13e5
to
577a8d9
Compare
577a8d9
to
b1b0226
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the interface much better now :)
The progress bar flashing is still a bit funky, I'll have a look at what's happening there.
I think the main issues have been resolved and I have markedly fewer comments ;)
Aside from the in-code comments:
- The y-axis label for the RM ANOVA doesn't seem to actually be added to the plot.
descriptivesTable$addFootnote( | ||
symbol = "<em>Note.</em>", | ||
message = sprintf( | ||
"Some combinations of factors are not observed and hence omitted (%d out of %d combinations are unobserved).", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need a different specifier than %d: "Formats d and i can also be used for logical variables, which will be converted to 0, 1 or NA."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to %g
because prod(...)
could return a double rather than an integer. I'm not sure why it's a problem that %d
accepts logicals, aside from integers. I'd rather see NA in a string than a crash in R. It's worthwhile pointing out that sprintf("%g", NA)
just returns "NA"
, same goes for paste
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I misread the thing I quoted. But %d complained here
posteriorPlotContainer$setOptionMustContainDependency("groupPosterior", options[["groupPosterior"]]) | ||
posteriorPlotContainer$dependOn( | ||
options = c("posteriorPlot", "modelTerms", "credibleInterval"), | ||
optionContainsValue = options["groupPosterior"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The syntax for this would be optionContainsValue = list(groupPosterior=options["groupPosterior"])
.
Although I think you could also just add it in options
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note the single brackets for indexing, options["groupPosterior"]
is shorthand for list(groupPosterior = options["groupPosterior"])
. Still it's better to just add it to options.
@@ -822,10 +825,10 @@ | |||
df <- data.frame(x = dd$x, y = dd$y) | |||
xName <- expression(R^2) | |||
plot$plotObject <- JASPgraphs::PlotPriorAndPosterior(dfLines = df, xName = xName, CRI = rsqCri, drawCRItxt = FALSE) | |||
plot$copyDependenciesFromJaspObject(jaspResults[["tableModelComparisonState"]]) | |||
plot$dependOn(jaspObject = jaspResults[["tableModelComparisonState"]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optionsFromObject
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
descriptivesPlotContainer$setOptionMustContainDependency("plotSeparateLines", options$plotSeparateLines) | ||
descriptivesPlotContainer$setOptionMustContainDependency("plotSeparatePlots", options$plotSeparatePlots) | ||
opts <- c("plotHorizontalAxis", "plotSeparateLines", "plotSeparatePlots") | ||
descriptivesPlotContainer$dependOn(optionContainsValue = options[opts]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be identical to descriptivesPlotContainer$dependOn(c("plotHorizontalAxis", "plotSeparateLines", "plotSeparatePlots"))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
posteriorPlotContainer$setOptionMustContainDependency("singleModelGroupPosterior", | ||
options[["singleModelGroupPosterior"]]) | ||
posteriorPlotContainer$dependOn(options = "singleModelPosteriorPlot", | ||
optionContainsValue = options["singleModelGroupPosterior"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value added in optionContainsValue
could just be set in options
: posteriorPlotContainer$dependOn(c("singleModelPosteriorPlot", "singleModelGroupPosterior"))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
Could you also resolve all the comments that you have incorporated so far? It's a little messy otherwise. |
Anything you wish to add @vandenman? If not, I'll merge this. |
@TimKDJ, I've got nothing further to add, go ahead! |
Changes to Bayesian Anova, Bayesian Ancova, and Bayesian repeated measures anova.
Overview of changes
Known issues/ bugs: