Add dedicated Number of Factors/Components analysis - #359
Conversation
|
👋 Friendly reminder: It looks like If your changes include bug fixes, new features, or UI tweaks, please consider adding a quick note to the |
9cf6ee5 to
832dad6
Compare
sisyphus-jasp
left a comment
There was a problem hiding this comment.
The new FA-based mode has a correctness issue in the scree plot.
- R/numberoffactors.R:192 — keep the real-data eigenvalue basis consistent when the parallel-analysis overlay is hidden. With
parallelAnalysisMethod = factorBased, the overlay path plotsparallelResult$fa.values, but turning off onlyscreePlotParallelAnalysisResultsswitches the real-data series to eigenvalues of the ordinary correlation matrix (principal-component eigenvalues) while the x-axis remains labeled ‘Factor’. That toggle can therefore change the apparent elbow/retention result instead of only hiding the simulated series. Please use the FA eigenvalues in this branch (or otherwise make the displayed basis consistent) and add coverage for FA mode with the overlay disabled.
sisyphus-jasp
left a comment
There was a problem hiding this comment.
Part 1/3: No blocking or important correctness defects were identified in this chunk. The new analysis consistently selects PC or FA eigenvalues and invalidates its cached parallel-analysis state through the parent container dependencies.
Part 2/3: No blocking or important correctness defects were identified in this patch chunk. The new analysis is registered, the removed PCA/EFA controls are paired with upgrade handling, and the tests cover the principal PC/FA eigenvalue-selection behavior introduced here.
Part 3/3: This chunk only updates PCA and EFA tests to reflect the removal of automatic factor/component retention options and related outputs. The replacement manual counts preserve the scenarios previously selected by those methods, and no concrete correctness defect is introduced here.
Findings
- [suggestion] R/numberoffactors.R:29 — Global multicore option is not restored Calling options(mc.cores = 1) permanently changes the R session setting, so unrelated analyses executed afterward may unexpectedly run single-threaded. Save the previous value and restore it with on.exit(), or pass the desired core count directly where supported.
Automated review by Sisyphus; findings are limited to changes in this pull request.
…s analysis - new analysis numberOfFactors: parallel analysis (PC/FA), eigenvalue threshold, summary table, PA table, scree plot; fa.parallel runs once, cached in state - EFA/PCA: retention reduced to manual IntegerField; PA table + scree plot options removed; dead code deleted - Upgrades.qml: remove stale options, msg pointing to new analysis - tests: PA/scree tests moved to test-numberoffactors.R; EFA/PCA tests use explicit manual counts (values unchanged); PA sim values need regeneration on first run (single fa.parallel call changes RNG path) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…actor count - PA sim values + suggestion regenerated (single seeded fa.parallel run) - orderLoadingsBy test: manualNumberOfFactors = 3 (was PA default) - new snapshot scree-plot-pc.svg (needs manual inspection) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Toggling screePlotParallelAnalysisResults off switched the real data series to PC eigenvalues while the axis still read 'Factor'. Always source the real data series from the cached parallel result. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DESCRIPTION 0.97.1 -> 0.98.0; without it files saved by this branch re-trigger the 0.97.1 step on every load (findClosestVersion uses >=). Upgrades.qml: msg on Upgrade element (form-level), guard ChangeJS on its own option, drop four dead ChangeRemove msgs -- ChangeRemove files them under the removed option name, which no longer has a control. Form-level and per-control upgrade messages do not currently render: AnalysisForm::bindTo reads them during Analyses::create(), before setUpgradeMsgs is called. Reported upstream. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0.16.4 files (2022) no longer open in JASP. New fixture saved with jaspFactor 0.95.5: EFA on parallelAnalysis, PCA on eigenvalues, so both the ChangeJS and all ChangeRemove entries fire. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c179b54 to
fac0c66
Compare
sisyphus-jasp
left a comment
There was a problem hiding this comment.
No blocking or important defects were found in the PR. The new Number of Factors/Components analysis passed its focused tests, the affected EFA tests passed, and the PCA/full-suite failures observed locally were reproduced on the baseline or caused by the local harness setup rather than by this PR.
Automated review by Sisyphus; findings are limited to changes in this pull request.
| if (ready) | ||
| .pcaCheckErrors(dataset, options, method = "numberOfFactors") | ||
|
|
||
| options(mc.cores = 1) # prevent the fa.parallel function using multiple cores by default |
There was a problem hiding this comment.
[suggestion] Restore mc.cores after forcing single-core parallel analysis
numberOfFactorsInternal calls options(mc.cores = 1) without restoring the previous value, so running this analysis leaves the R session in single-core mode for later analyses. Save the previous option and restore it with on.exit(), or pass the core count directly where supported.
sisyphus-jasp
left a comment
There was a problem hiding this comment.
The new Number of Factors/Components analysis has a user-visible retention inconsistency: the summary can report one parallel-analysis count while the parallel-analysis table marks a different number of factors/components as retained.
Findings
- [important] R/numberoffactors.R:153 — Keep parallel-analysis retention markers consistent with the summary The summary uses psych::fa.parallel's ncomp/nfact as the parallel-analysis suggestion, but the table marks retained rows by comparing realDataEigen to pc.sim/fa.sim. psych::fa.parallel defaults to quant = .95 for ncomp/nfact, while pc.sim/fa.sim are the returned mean simulated eigenvalues shown in this table, so the two criteria can disagree. With the new default test data, the summary reports PC-based parallel analysis suggests 2 components, but the table marks Component 1, 2, and 3 with '*' as retained. Users now get contradictory retention advice in the same analysis output; make the table markers and summary count use the same criterion or label the displayed comparison differently.
Automated review by Sisyphus; findings are limited to changes in this pull request.
Table starred rows where real > mean simulated eigenvalue; summary used psych ncomp/nfact (95th percentile criterion), so the two could disagree. Both now use shared .nofParallelSuggested(); footnote clarifies displayed simulated eigenvalues are means. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
542ae27 to
d414f35
Compare
|
🚀 I’ve started reviewing commit |
sisyphus-jasp
left a comment
There was a problem hiding this comment.
Found a correctness issue in the new Number of Factors/Components analysis: zero parallel-analysis suggestions are forced to one, so the summary and table can tell users to retain a factor/component that psych::fa.parallel did not suggest.
Automated review by Sisyphus; findings are limited to changes in this pull request.
| count <- if (pcBased) parallelResult$ncomp else parallelResult$nfact | ||
| if (is.na(count)) | ||
| count <- length(if (pcBased) parallelResult$pc.values else parallelResult$fa.values) | ||
| return(max(1, count)) |
There was a problem hiding this comment.
[important] Do not force zero parallel-analysis suggestions to one
.nofParallelSuggested() wraps psych::fa.parallel's ncomp/nfact in max(1, count), so cases where parallel analysis suggests 0 retained factors/components are reported as 1 and the first row is starred. For example, running the new analysis on contNormal and contGamma with FA-based parallel analysis prints psych's 'number of factors = 0', but the summary shows 1 and the table marks 'Factor 1*'. This gives contradictory, incorrect retention advice in the new dedicated retention analysis.
.nofParallelSuggested forced max(1, count), so a 0 suggestion from psych::fa.parallel showed as 1 with Factor 1 starred. Return count as-is; 0 now yields summary count 0 and no starred rows. Also restore previous mc.cores option via on.exit instead of leaving session single-core. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1363fbf to
4f7ba19
Compare
|
🚀 I’ve started reviewing commit |
sisyphus-jasp
left a comment
There was a problem hiding this comment.
No blocking or important defects found. The new Number of Factors/Components analysis and affected EFA coverage passed locally; PCA and full-suite failures were reproduced on the baseline and are not attributable to this PR. One non-blocking help-text mismatch remains.
Automated review by Sisyphus; findings are limited to changes in this pull request.
| Group | ||
| { | ||
| title: qsTr("Parallel Analysis") | ||
| info: qsTr("Parallel analysis compares the eigenvalues of the data to eigenvalues of simulated random data. Factors/components are suggested for retention when their eigenvalue is greater than the mean eigenvalue of the simulated data.") |
There was a problem hiding this comment.
[suggestion] Align parallel-analysis help text with the reported criterion
The new analysis now uses psych::fa.parallel's ncomp/nfact suggestion, which is based on the 95th percentile of simulated eigenvalues, and the table footnote says so. This info text still tells users factors/components are suggested when the real eigenvalue exceeds the mean simulated eigenvalue, which can contradict the unstarred rows users see in the table. Updating the help text would keep the UI documentation aligned with the output.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🚀 I’ve started reviewing commit |
sisyphus-jasp
left a comment
There was a problem hiding this comment.
No blocking or important defects were found. The focused Number of Factors/Components coverage and affected EFA tests passed; PCA and full-suite failures were reproduced on the baseline, and one harness invocation failure was due to local test setup rather than the PR.
Automated review by Sisyphus; findings are limited to changes in this pull request.
Note for the core team: upgrade messages in
|
New analysis: Number of Factors/Components
Consolidates parallel analysis (PC- and FA-based), the eigenvalue criterion, and the scree plot into one dedicated analysis, usable for both EFA and PCA decisions.
Provides a summary table with suggested retention numbers from each method side-by-side.
The eigenvalue/PA-based factor count options are removed from both analyses. The number of factors/components is now set manually, with an info tip pointing users to the new analysis.
This removes the ambiguity of running a full factor model and factor retention simultaneously.
fixes [Feature Request]: Document if the Eigenvalue criterion is based on the principal component jasp-issues#3970:
Previously the eigenvalue criterion always used PC eigenvalues (pc.values), even when FA-based parallel analysis was selected. This caused confusing results in EFA where factors with factor eigenvalues < 1 were retained because the threshold was applied to PC eigenvalues.
Now the eigenvalue criterion uses PC eigenvalues when PC-based PA is selected and factor eigenvalues when FA-based PA is selected.
A footnote on the summary table states which eigenvalue type was used (e.g., "Eigenvalue criterion based on factor eigenvalues.").
The info text for the threshold field explains the link to the PA method selection.
For testing:
test.jasp.zip