fix4163#455
Conversation
There was a problem hiding this comment.
Pull request overview
Improves robustness and output correctness in the Process Capability Studies analysis (Quality Control module), specifically around 3-parameter Weibull fitting and Anderson–Darling p-value display, and adds regression coverage for the reported issue.
Changes:
- Adjusts 3-parameter Weibull start-value handling to fall back to fixed defaults when start estimation fails.
- Fixes Anderson–Darling p-value column typing so it renders as a p-value (not an integer).
- Adds a new verification test case (incl. dataset + plot snapshot) for 3-parameter Weibull non-normal capability analysis.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
R/commonQualityControl.R |
Makes 3-parameter Weibull fitting more resilient by falling back to default starting values. |
R/processCapabilityStudies.R |
Changes AD test p-value column type to pvalue to improve display/formatting. |
tests/testthat/test-processCapabilityStudies.R |
Adds regression/verification tests and a plot snapshot assertion for the 3-parameter Weibull case. |
tests/testthat/datasets/processCapabilityStudy/realDataExample1.csv |
Adds a real-data CSV used by the new test. |
tests/testthat/_snaps/processCapabilityStudies/capability-of-the-processl55-subplot-1.svg |
Adds the expected plot snapshot for the new test case. |
| } | ||
| table$addColumnInfo(name = "ad", title = gettext("AD"), type = "integer") | ||
| table$addColumnInfo(name = "p", title = gettext("<i>p</i>-value"), type = "integer") | ||
| table$addColumnInfo(name = "p", title = gettext("<i>p</i>-value"), type = "pvalue") |
There was a problem hiding this comment.
p is now declared as type pvalue, but later (when nStages > 1) the code sets changeDf$p <- "-", which will coerce the entire p column to character during rbind(). That prevents p-value formatting and may break pvalue rendering. Consider keeping p numeric (e.g., use NA_real_ for change rows and leave the cell blank / add a footnote) so the pvalue column type can work consistently.
| table$addColumnInfo(name = "p", title = gettext("<i>p</i>-value"), type = "pvalue") | |
| table$addColumnInfo(name = "p", title = gettext("<i>p</i>-value"), type = "string") |
| test_that("LF55.1 Additional 3-paraneter Weibull verification - Non-conformance statistics table results match", { | ||
| table <- results[["results"]][["capabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis_PerformanceNonNormal"]][["data"]] | ||
| jaspTools::expect_equal_tables(table, | ||
| list(0, 0, "ppm < LSL", 25245.93, 23529.41, "ppm > USL", 25245.93, | ||
| 23529.41, "Total ppm")) | ||
| }) | ||
|
|
||
| test_that("LF55.2 Additional 3-paraneter Weibull verification - Capability of the process plot matches", { | ||
| plotName <- results[["results"]][["capabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis_capabilityPlot"]][["data"]] | ||
| testPlot <- results[["state"]][["figures"]][[plotName]][["obj"]] | ||
| jaspTools::expect_equal_plots(testPlot, "capability-of-the-processL55") | ||
| }) | ||
|
|
||
| test_that("LF55.3 Additional 3-paraneter Weibull verification - Process performance (total) table results match", { | ||
| table <- results[["results"]][["capabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis_overallCapabilityNonNormal"]][["data"]] | ||
| jaspTools::expect_equal_tables(table, | ||
| list("<unicode>", 0.65, "<unicode>", 0.65)) | ||
| }) | ||
|
|
||
| test_that("LF55.4 Additional 3-paraneter Weibull verification - Process summary table results match", { |
There was a problem hiding this comment.
The new test descriptions contain a typo (“3-paraneter”). This makes it harder to search/compare with other LFxx test labels; please correct the spelling in these test_that() titles (and any other occurrences in this added block).
| test_that("LF55.1 Additional 3-paraneter Weibull verification - Non-conformance statistics table results match", { | |
| table <- results[["results"]][["capabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis_PerformanceNonNormal"]][["data"]] | |
| jaspTools::expect_equal_tables(table, | |
| list(0, 0, "ppm < LSL", 25245.93, 23529.41, "ppm > USL", 25245.93, | |
| 23529.41, "Total ppm")) | |
| }) | |
| test_that("LF55.2 Additional 3-paraneter Weibull verification - Capability of the process plot matches", { | |
| plotName <- results[["results"]][["capabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis_capabilityPlot"]][["data"]] | |
| testPlot <- results[["state"]][["figures"]][[plotName]][["obj"]] | |
| jaspTools::expect_equal_plots(testPlot, "capability-of-the-processL55") | |
| }) | |
| test_that("LF55.3 Additional 3-paraneter Weibull verification - Process performance (total) table results match", { | |
| table <- results[["results"]][["capabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis_overallCapabilityNonNormal"]][["data"]] | |
| jaspTools::expect_equal_tables(table, | |
| list("<unicode>", 0.65, "<unicode>", 0.65)) | |
| }) | |
| test_that("LF55.4 Additional 3-paraneter Weibull verification - Process summary table results match", { | |
| test_that("LF55.1 Additional 3-parameter Weibull verification - Non-conformance statistics table results match", { | |
| table <- results[["results"]][["capabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis_PerformanceNonNormal"]][["data"]] | |
| jaspTools::expect_equal_tables(table, | |
| list(0, 0, "ppm < LSL", 25245.93, 23529.41, "ppm > USL", 25245.93, | |
| 23529.41, "Total ppm")) | |
| }) | |
| test_that("LF55.2 Additional 3-parameter Weibull verification - Capability of the process plot matches", { | |
| plotName <- results[["results"]][["capabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis_capabilityPlot"]][["data"]] | |
| testPlot <- results[["state"]][["figures"]][[plotName]][["obj"]] | |
| jaspTools::expect_equal_plots(testPlot, "capability-of-the-processL55") | |
| }) | |
| test_that("LF55.3 Additional 3-parameter Weibull verification - Process performance (total) table results match", { | |
| table <- results[["results"]][["capabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis"]][["collection"]][["capabilityAnalysis_nonNormalCapabilityAnalysis_overallCapabilityNonNormal"]][["data"]] | |
| jaspTools::expect_equal_tables(table, | |
| list("<unicode>", 0.65, "<unicode>", 0.65)) | |
| }) | |
| test_that("LF55.4 Additional 3-parameter Weibull verification - Process summary table results match", { |
| @@ -0,0 +1,171 @@ | |||
| data | |||
There was a problem hiding this comment.
The CSV header includes a UTF-8 BOM character before data (it renders as an invisible/odd character). This can cause the column name to be read as something other than data on some systems, which would break tests/options that refer to "data". Please save this file without a BOM so the header is exactly data.
| data | |
| data |
Kucharssim
left a comment
There was a problem hiding this comment.
The code looks fine to me, I can confirm that the error in the issue is fixed.
Uh oh!
There was an error while loading. Please reload this page.