[jaspDescriptives] Add ECDF (Empirical Cumulative Distribution Function) plot option...#463
[jaspDescriptives] Add ECDF (Empirical Cumulative Distribution Function) plot option...#463sisyphus-jasp wants to merge 3 commits intojasp-stats:masterfrom
Conversation
…on) plot option...
FBartos
requested review from
Copilot and
vandenman
and removed request for
vandenman
There was a problem hiding this comment.
Pull request overview
This PR adds an ECDF (Empirical Cumulative Distribution Function) plot option to the Descriptives module in jaspDescriptives. The implementation follows patterns from existing plot options, adding a new checkbox in the QML interface and corresponding R functions to generate the plots.
Changes:
- Added a new ECDF plot checkbox in the Basic Plots section of the Descriptives UI
- Added R wrapper function parameter
ecdfPlot = FALSE - Implemented
.descriptivesEcdfPlot()function to generate ECDF plots using ggplot2
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| inst/qml/Descriptives.qml | Added checkbox for "Empirical cumulative distribution" plot option in the Basic Plots section |
| R/descriptivesWrapper.R | Added default parameter ecdfPlot = FALSE to the Descriptives function signature |
| R/descriptives.R | Added main logic to handle ECDF plots and implemented .descriptivesEcdfPlot() helper function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ecdfPlots[[var]] <- .descriptivesEcdfPlot(splitDat, options, var) | ||
| } else { | ||
| ecdfPlots[[var]] <- .descriptivesEcdfPlot(dataset, options, var) | ||
| } | ||
| ecdfPlots[[var]]$dependOn(optionContainsValue = list(variables = var)) |
There was a problem hiding this comment.
The ECDF plot implementation doesn't properly handle split datasets. When makeSplit is true, splitDat is a list of data frames (one per split level), but .descriptivesEcdfPlot expects a single data frame.
Following the pattern from .descriptivesFrequencyPlots (lines 1169-1210) and .descriptivesPieChart (lines 1908-1933), the .descriptivesEcdfPlot function should check if options$splitBy is not empty, and if so, iterate through the split levels, creating a container with multiple plots. Alternatively, follow the QQ plots pattern (lines 180-200) where the split handling is done in the main function.
The simplest fix is to modify .descriptivesEcdfPlot to handle split datasets internally like .descriptivesFrequencyPlots does.
| ecdfPlots[[var]] <- .descriptivesEcdfPlot(splitDat, options, var) | |
| } else { | |
| ecdfPlots[[var]] <- .descriptivesEcdfPlot(dataset, options, var) | |
| } | |
| ecdfPlots[[var]]$dependOn(optionContainsValue = list(variables = var)) | |
| # When splitBy is used, create a container per variable and add one ECDF plot per split level | |
| splitContainer <- createJaspContainer(var) | |
| splitContainer$dependOn(optionContainsValue = list(variables = var)) | |
| for (splitLevel in names(splitDat)) { | |
| splitData <- splitDat[[splitLevel]] | |
| ecdfPlot <- .descriptivesEcdfPlot(splitData, options, var) | |
| ecdfPlot$dependOn(optionContainsValue = list(variables = var)) | |
| splitContainer[[splitLevel]] <- ecdfPlot | |
| } | |
| ecdfPlots[[var]] <- splitContainer | |
| } else { | |
| ecdfPlots[[var]] <- .descriptivesEcdfPlot(dataset, options, var) | |
| ecdfPlots[[var]]$dependOn(optionContainsValue = list(variables = var)) | |
| } |
| if (is.null(jaspResults[["ecdfPlot"]])) { | ||
| jaspResults[["ecdfPlot"]] <- createJaspContainer(gettext("Empirical Cumulative Distribution")) | ||
| jaspResults[["ecdfPlot"]]$dependOn(c("ecdfPlot", "splitBy", "variables")) | ||
| jaspResults[["ecdfPlot"]]$position <- 5 |
There was a problem hiding this comment.
Position conflict: This ECDF container is assigned position 5, which is the same position already used by Distribution Plots (line 107). Each container should have a unique position to ensure proper ordering in the output. Consider using position 5.5 or adjusting the positions of subsequent containers (Box plots at 7, QQ plots at 8, etc.).
| jaspResults[["ecdfPlot"]]$position <- 5 | |
| jaspResults[["ecdfPlot"]]$position <- 5.5 |
| ggplot2::stat_ecdf(geom = "step", colour = "black") + | ||
| jaspGraphs::drawAxis(xName = variable, yName = gettext("Cumulative Proportion")) + | ||
| ggplot2::theme(panel.grid = ggplot2::element_blank()) |
There was a problem hiding this comment.
This should use jaspGraphs::geom_rangeframe() and jaspGraphs::themeJaspRaw(). Also, jaspGraphs::drawAxis is deprecated in favor of ggplot2::scale_[x|y]_continuous
|
Sisyphus is working on this revision. I picked up the developer feedback and will post an update here when done (usually within 1 hour). |
Revision pushed by SisyphusStatus: FIXED Automated review: Approved after 1 iteration(s). SummaryAddressed feedback
Verification
Remaining
Implementation Plan
Expected test impact
Test Results
Developer feedback addressedRevision Request for PR #463ReviewsReview by @copilot-pull-request-reviewer[bot] (COMMENTED) - 2026-02-26 12:01 UTC
|
|
Sisyphus is working on this revision. I picked up the developer feedback and will post an update here when done (usually within 1 hour). |
Revision pushed by SisyphusStatus: FIXED Automated review: Approved after 1 iteration(s). SummaryAddressed feedback
Changes made
Verification
Remaining items
Implementation PlanRevision plan for PR #463
Test Results
Developer feedback addressedRevision Request for PR #463ReviewsReview by @copilot-pull-request-reviewer[bot] (COMMENTED) - 2026-02-26 12:01 UTC
|
Summary
Fixes: https://github.com/jasp-stats/INTERNAL-jasp/issues/3007
PR Summary: ECDF Plot Option for Descriptives Module
What was done
Added ECDF (Empirical Cumulative Distribution Function) plot option to the Descriptives module, following the pattern from the Distribution module.
Changes Made
QML (inst/qml/Descriptives.qml):
ecdfPlotR Wrapper (R/descriptivesWrapper.R):
ecdfPlot = FALSEto the options listR Implementation (R/descriptives.R):
ecdfPlotoption in the main function.descriptivesEcdfPlot()function that generates ECDF plots usingggplot2::stat_ecdf()Test Results
Verification
Implementation Plan
Implementation Plan: ECDF Plot Option for Descriptives Module
Root Cause
The Descriptives module lacks an ECDF (Empirical Cumulative Distribution Function) plot option. This functionality exists in the Distribution module and needs to be added to Descriptives for consistency.
Proposed Changes
1. QML Changes (inst/qml/Descriptives.qml)
ecdfPlotqsTr("Empirical cumulative distribution")2. R Changes (R/descriptives.R)
ecdfPlotoption in the main function.descriptivesEcdfPlot()to generate ECDF plots usingggplot2::stat_ecdf()ecdfPlot,splitBy,variablesImplementation Pattern
Follow the existing pattern used for distributionPlots:
if (options$ecdfPlot)createJaspContainer(gettext("Empirical Cumulative Distribution"))$dependOn(c("ecdfPlot", "splitBy", "variables"))Expected Test Impact
Test Results
Automated Code Review
Approved after 1 review iteration(s).