-
Notifications
You must be signed in to change notification settings - Fork 20
Fix mg error #286
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
base: master
Are you sure you want to change the base?
Fix mg error #286
Conversation
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.
Pull Request Overview
This PR fixes errors in the confirmatory factor analysis module and adds comprehensive tests for standardization and invariance testing functionality. The fix addresses an issue referenced in a related GitHub issue while expanding test coverage to ensure standardization and group invariance testing work correctly.
- Fixes confirmatory factor analysis error handling and functionality
- Adds extensive tests for factor loadings with bootstrapping, standardization and group invariance
- Updates data handling functions for better covariance matrix processing
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-confirmatoryfactoranalysis.R | Adds comprehensive test cases for standardization and invariance testing scenarios |
| jaspFactor.Rproj | Updates project ID for the R project file |
| R/principalcomponentanalysis.R | Improves covariance matrix data handling and processing |
| R/exploratoryfactoranalysis.R | Updates option references and data reading flow |
| R/confirmatoryfactoranalysis.R | Removes deprecated bootstrap function and fixes mean structure condition |
| } else { | ||
| dataset[] <- lapply(dataset, function(x) as.numeric(as.character(x))) | ||
| return(dataset) | ||
| return(.readDataSetToEnd(columns.as.numeric = unlist(options$variables))) |
Copilot
AI
Jul 22, 2025
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 line appears to be duplicated logic from the .pcaAndEfaHandleData function. Consider consolidating this data reading logic to avoid code duplication.
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.
it is already in a common function
| # it seems the column names are sorted alphabetically when all columns are read | ||
| # so we need to sort the column names to match the order of the variables | ||
| sortedIndices <- sort(as.numeric(gsub(".*_(\\d+)_.*", "\\1", colnames(dataset)))) | ||
| sortedNames <- paste0("JaspColumn_", sortedIndices, "_Encoded") | ||
| dataset <- dataset[, sortedNames] |
Copilot
AI
Jul 22, 2025
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 comment and the following sorting logic is duplicated from .pcaAndEfaDataCovarianceCheck function. Consider extracting this into a shared utility function to reduce code duplication.
| # it seems the column names are sorted alphabetically when all columns are read | |
| # so we need to sort the column names to match the order of the variables | |
| sortedIndices <- sort(as.numeric(gsub(".*_(\\d+)_.*", "\\1", colnames(dataset)))) | |
| sortedNames <- paste0("JaspColumn_", sortedIndices, "_Encoded") | |
| dataset <- dataset[, sortedNames] | |
| dataset <- .sortDatasetColumns(dataset, options$variables) |
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 code is already in a common function shared between PCA and EFA.
R/exploratoryfactoranalysis.R
Outdated
| .efaComputeResults <- function(modelContainer, dataset, options, ready) { | ||
|
|
||
| corMethod <- switch(options[["baseDecompositionOn"]], | ||
| corMethod <- switch(options[["analysisBasedOn"]], |
Copilot
AI
Jul 22, 2025
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 option name changed from 'baseDecompositionOn' to 'analysisBasedOn'. Ensure this change is intentional and that all related code and documentation are updated consistently.
| if (options$meanStructure || options$group != "") { | ||
|
|
Copilot
AI
Jul 22, 2025
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.
[nitpick] The condition has been expanded to include group analysis. Consider adding a comment explaining why intercepts are needed when group analysis is performed, as this may not be immediately obvious to future maintainers.
| if (options$meanStructure || options$group != "") { | |
| # Intercepts are included when group analysis is performed (options$group != "") | |
| # to account for group-level differences in the model. This ensures that the | |
| # model properly estimates group-specific means for the latent variables. | |
| if (options$meanStructure || options$group != "") { |
530354c to
34b8b9d
Compare
the error is fixed with this PR: jasp-stats/jaspSem#318
but this adds more tests to check that standardization and invariance testing works