Enhance relationship matrices and BLUP workflow - #10
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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
| # 更新残差方差 | ||
| residuals = y - X * solutions[1:n_fixed] - sum(Z * solutions[n_fixed+1:end] for (name, Z) in Z_dict) |
There was a problem hiding this comment.
Slice random-effect coefficients when forming residuals
The residual term in the EM loop subtracts sum(Z * solutions[n_fixed+1:end] for (name, Z) in Z_dict), i.e. every random-effect design matrix is multiplied by the entire concatenated random-effect solution vector. As soon as the model contains more than one random effect, each Z expects only its own coefficient slice, so this call either raises a dimension-mismatch error or reuses the same coefficients for all effects. The code should accumulate residuals by iterating over the random effects with a running offset, mirroring the logic used later in the loop.
Useful? React with 👍 / 👎.
| if effect.type == :additive && !isnothing(dm.A_inv_matrix) | ||
| u_Ainv_u = dot(u, dm.A_inv_matrix * u) | ||
| tr_term_u = tr(C_inv[current_pos+1:end, current_pos+1:end] * dm.A_inv_matrix) # 简化 | ||
| variances_new[name] = (u_Ainv_u + tr_term_u) / dim |
There was a problem hiding this comment.
Trace term addresses wrong submatrix for subsequent random effects
When updating a random effect’s variance, the trace term is computed as tr(C_inv[current_pos+1:end, current_pos+1:end] * dm.A_inv_matrix). For any model with more than one random effect, this expression takes the entire remaining lower-right block of C_inv and multiplies it with the full A⁻¹, producing a dimension mismatch or an incorrect trace for all but the first random effect. The slice should be limited to the current effect’s block (current_pos+1:current_pos+dim) and use the corresponding covariance structure for that effect only.
Useful? React with 👍 / 👎.
Summary
Testing
julia --project=AnimalBreeding.jl -e 'using Pkg; Pkg.test()'(fails:juliaexecutable not available in container)https://chatgpt.com/codex/tasks/task_e_68de0a88d0dc8321a45602074af76510