Fix multiple bugs: division by zero, undefined variable, edge cases#180
Open
bbuchsbaum wants to merge 4 commits into
Open
Fix multiple bugs: division by zero, undefined variable, edge cases#180bbuchsbaum wants to merge 4 commits into
bbuchsbaum wants to merge 4 commits into
Conversation
- Fix division by zero in Huber weighting (fmri_robust_fitting.R) When residuals are exactly 0, abs(u) would be 0 causing division by zero. Added protection using pmax with machine epsilon. - Fix undefined variable xf in estimate_hrf (fmri_betas.R) The variable xf was used in predict() but its definition was commented out, causing runtime errors when has_fixed=TRUE. Properly handle both has_fixed=TRUE and has_fixed=FALSE cases. - Fix bootstrap block size edge case (bootstrap.R) When nrows < block_size, nblocks becomes 0 and rep(1:0, ...) causes unexpected behavior. Now handles this by using a single block when nblocks < 1. - Change silent zero fallback to warning with NA (fmri_betas.R) When mixed model solvers fail, returning zeros silently could produce misleading results. Now returns NA_real_ with warning to make failures visible. - Remove debug code and commented browser() statements
- Add get_ar_order() helper in fmri_lm_config.R to centralize AR order extraction from config, replacing 12+ duplicated switch statements across the codebase - Remove ~80 lines of commented-out generic definitions in all_generic.R, consolidating import notes into single block at top of file - Net reduction of 113 lines through DRY improvements Files affected: fmrilm.R, fmri_lm_runwise.R, fmri_lm_chunkwise.R, fmri_lm_strategies.R, fmri_lm_config.R, all_generic.R https://claude.ai/code/session_01BkYFTd6Q7J1A4UozMa6TEw
Major improvements: - Add compute_chunk_stats() helper to unify beta stats + contrast computation across runwise and chunkwise paths - Add validation helpers (check_required_fields, check_field_type) for group_data validation - Add utility helpers (is_empty_indices, as_indices, ensure_matrix, etc.) - Add named constants for magic numbers (.DEFAULT_CHUNK_SIZE_*, etc.) Bug fixes: - Fix return type inconsistency in pool_runwise_results() - now always returns list of contrast tibbles - Standardize null/empty index handling in fmri_betas.R Code reduction: - Remove duplicated beta_stats_matrix + fit_lm_contrasts_fast calls - Consolidate validation logic in group_data files - Use consistent patterns for type checking and null handling https://claude.ai/code/session_01BkYFTd6Q7J1A4UozMa6TEw
- Add build_fmri_lm_cfg() helper in fmri_lm_config.R that consolidates all config building logic with backward compatibility parameter handling - Replace ~140 lines of duplicated config building code in both fmri_lm.formula and fmri_lm.fmri_model with calls to helper - Net reduction of ~15 lines with cleaner, more maintainable code https://claude.ai/code/session_01BkYFTd6Q7J1A4UozMa6TEw
Owner
Author
|
@copilot resolve the merge conflicts in this pull request |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79b52fd10c
ℹ️ 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".
| # conres_list[[1]] is already a list of contrast tibbles | ||
| list( | ||
| contrasts = combined_contrasts, # Single tibble with all contrasts | ||
| contrasts = conres_list[[1]], # List of contrast tibbles (same as meta_contrasts) |
There was a problem hiding this comment.
Return contrasts in the same tabular structure as the multi-run branch: this line now returns the raw list (conres_list[[1]]) instead of a bound tibble, so single-run outputs have a different type than multi-run outputs. Downstream consumers expect x$result$contrasts to be a data frame (e.g., pull_stat() filters on type), so runwise fits with only one run will fail when users request contrast coefficients/statistics.
Useful? React with 👍 / 👎.
Owner
Author
|
@codex resolve the merge conflicts in this pull request |
Summary
Testing
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix division by zero in Huber weighting (fmri_robust_fitting.R)
When residuals are exactly 0, abs(u) would be 0 causing division
by zero. Added protection using pmax with machine epsilon.
Fix undefined variable xf in estimate_hrf (fmri_betas.R)
The variable xf was used in predict() but its definition was
commented out, causing runtime errors when has_fixed=TRUE.
Properly handle both has_fixed=TRUE and has_fixed=FALSE cases.
Fix bootstrap block size edge case (bootstrap.R)
When nrows < block_size, nblocks becomes 0 and rep(1:0, ...)
causes unexpected behavior. Now handles this by using a single
block when nblocks < 1.
Change silent zero fallback to warning with NA (fmri_betas.R)
When mixed model solvers fail, returning zeros silently could
produce misleading results. Now returns NA_real_ with warning
to make failures visible.
Remove debug code and commented browser() statements