Distribution comparison improvements#237
Distribution comparison improvements#237Kucharssim wants to merge 8 commits intojasp-stats:masterfrom
Conversation
|
Not sure what that is about. I am also not sure if that was there before. |
|
The "Full distribution name" box looks a bit lost. Maybe group it with the comparison table? Potentially also rename it from "name"-> "specification" or something. For a second I was wondering that a full name would be. |
|
One very small detail that is surely not a showstopper would be to not have the output table recreate when I click the "+". In that case I havent even added another distribution but the table recreates regardless. I am not entirely certain how this could be facilitated. So you can also leave this as is for now. |
juliuspfadt
left a comment
juliuspfadt
left a comment
There was a problem hiding this comment.
This already looked very good before. The error is the only thing that really needs fixing. The rest is optional. And maybe whatever copilot comes up with.
There was a problem hiding this comment.
Pull request overview
This PR improves the “Compare Continuous Distributions” analysis by refactoring how per-distribution results are computed/displayed, adding UI affordances (including a “Full distribution name” toggle and an icon), and updating snapshots/tests to match the new output formatting and result keys.
Changes:
- Refactors distribution computation/comparison to cache per-distribution fit + information criteria, then derives ranking/weights from those results.
- Updates UI: allows an explicit “no selection” state for distributions and adds a “Full distribution name” option.
- Updates tests/snapshots and module metadata (icon + capitalization + version bump).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
R/compareDistributions.R |
Refactor of comparison pipeline, new distribution naming logic, new per-distribution container keys. |
R/compareDistributionsWrapper.R |
Wrapper/docs updates for new/changed options and version string. |
inst/qml/CompareContinuousDistributions.qml |
Adds “Full distribution name” checkbox. |
inst/qml/common/DistributionSpecification.qml |
Allows empty distribution selection + placeholder; hides settings for empty selection too. |
inst/Description.qml |
UI title capitalization + new icon reference. |
inst/icons/discoverdistributions-compareDistributions.svg |
New icon asset. |
tests/testthat/test-comparecontinuousdistributions.R |
Updates result paths and snapshot formatting width. |
tests/testthat/_snaps/comparecontinuousdistributions.md |
Snapshot updated for new comparison-table output formatting/names. |
DESCRIPTION |
Package version bump to 0.96.1. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
R/compareDistributionsWrapper.R
Outdated
| #' @param distributions, Specify distributions to be compared... | ||
| #' @param empiricalPlots, Outputs histogram vs theoretical density plot, empirical vs. theoretical cumulative distribution function, the Q-Q plot, and the P-P plot. | ||
| #' Defaults to \code{FALSE}. | ||
| #' Defaults to \code{TRUE}. | ||
| #' @param empiricalPlotsCi, Add the confidence interval to the P-P and Q-Q plots. | ||
| #' Defaults to \code{FALSE}. | ||
| #' @param goodnessOfFit, Compute goodness of fit tests. For most of the distributions, the default tests are Cramér von-Misses and Anderson-Darling for composite null hypothesis. Note that these tests rely on randomly splitting the data in two sets; as a result, the results may be variable, especially for small sample sizes. When a distribution does not have free parameters (i.e., all parameters are fixed), the tests are Kolmorogov-Smirnov, and Cramér von-Misses and Anderson-Darling for simple null hypothesis. For normal distributions with free location and scale parameters, specific versions of goodness of fit tests are computed, appropriate for this setting. If the normal distribution has some parameters fixed, it is treated as any other distribution. | ||
| #' Defaults to \code{FALSE}. | ||
| #' Defaults to \code{TRUE}. | ||
| #' @param fullDistributionName, Displays the full distribution name, including parameter specification. If unchecked, names are shortened. | ||
| #' Defaults to \code{TRUE}. | ||
| #' @param goodnessOfFit, Compute goodness of fit tests. For most of the distributions, the default tests are Cramér-von Mises and Anderson-Darling for composite null hypothesis. Note that these tests rely on randomly splitting the data in two sets; as a result, the results may be variable, especially for small sample sizes. When a distribution does not have free parameters (i.e., all parameters are fixed), the tests are Kolmorogov-Smirnov, and Cramér-von Mises and Anderson-Darling for simple null hypothesis. For normal distributions with free location and scale parameters, specific versions of goodness of fit tests are computed, appropriate for this setting. If the normal distribution has some parameters fixed, it is treated as any other distribution. | ||
| #' Defaults to \code{TRUE}. |
There was a problem hiding this comment.
The roxygen docs claim several options default to TRUE (empiricalPlots, empiricalPlotsCi, fullDistributionName, goodnessOfFit), but the function signature defaults them to FALSE. Please align the documentation with the actual defaults (or update the defaults if the docs are correct), otherwise users and generated help pages will be misleading.
There was a problem hiding this comment.
This is correct, but a curious since the wrapper was generated automatically. @boutinb any idea why that may happen? I may have clicked on the "generate wrapper" button after playing around with some of the options, are the generated wrapper doc strings generated from the defaults or from whatever is set at the time the button is clicked?
Not a big issue, I will just regen the wrapper. Just thought you might want to see this @boutinb
|
Thanks @juliuspfadt! Great that you caught the bug, I wouldn't have noticed... I addressed your comments as well as Copilot's suggestions, except for this:
I would have to make the dependency structure of the table much more complicated to fix this, I think. In any case, the results are actually not recomputed, only the table gets recreated. Since you mentioned it's not a showstopper, I am inclined to merge this as is (provided the tests pass). |
fixes https://github.com/jasp-stats/INTERNAL-jasp/issues/3152
fixes https://github.com/jasp-stats/external-issues/issues/9
fixes https://github.com/jasp-stats/external-issues/issues/10
fixes https://github.com/jasp-stats/external-issues/issues/2 (point 1, point 2 was fixed by jasp-stats/jasp-desktop#6162 and will not be visible on the current release yet)