-
Notifications
You must be signed in to change notification settings - Fork 6
Format ov names in formatted objects #122
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
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 755f0600f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| keepRn <- rep(TRUE, length(rn)) | ||
| keepRn[isTempRn][isDupTempRn] <- FALSE | ||
| mat <- mat[keepRn, , drop = FALSE] |
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.
Fix temp-OV duplicate filtering (current subsetting is no-op)
In R, chained indexing like keepRn[isTempRn][isDupTempRn] <- FALSE updates a temporary vector, so keepRn is unchanged. When both .TEMP_OV__x and x rows exist, the temp rows are not dropped, and rnClean[keepRn] then collapses both to x, yielding duplicated row names/cols. That can lead to ambiguous indexing or downstream errors when consumers expect unique names (e.g., modsem_inspect/modsem_predict output). You’ll want to index keepRn directly (e.g., keepRn[which(isTempRn)[isDupTempRn]] <- FALSE), and the same pattern appears in the column and vector cases.
Useful? React with 👍 / 👎.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fa75f8b14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
R/inspect_da.R
Outdated
| partable = modsemParTable(model$parTable), | ||
| coefficients.all = .modsemVector(model$coefs.all), | ||
| coefficients.free = .modsemVector(model$coefs.free), | ||
| partable = parameter_estimates(model), |
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.
Honor is.public when returning partable
modsem_inspect_da() now builds partable via parameter_estimates(model) without forwarding is.public, but parameter_estimates.modsem_da defaults is.public = TRUE (R/generics_modsem_da.R:10–27), which formats names and drops temp OV rows. As a result, modsem_inspect(..., is.public = FALSE) returns an internal set of matrices/vectors alongside a public partable, so name-based joins (e.g., matching partable$lhs/rhs to lambda/theta from the same inspect call) can fail due to mismatched identifiers. Passing is.public = is.public here would keep the payload consistent.
Useful? React with 👍 / 👎.
No description provided.