-
Notifications
You must be signed in to change notification settings - Fork 58
Add robust inverse of a FI matrix in CBMR inference #933
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: main
Are you sure you want to change the base?
Conversation
…using SVD and a thresholding approach to ignore small singular values
Reviewer's GuideThis PR adds a numerically stable inverse function for Fisher information matrices via SVD thresholding and integrates it into the CBMR inference pipeline, replacing direct matrix inversions and updating related imports, docs, and tests. Class diagram for robust_inverse utility functionclassDiagram
class robust_inverse {
+robust_inverse(FI: np.ndarray, eps: float = 1e-8) np.ndarray
}
class np.linalg {
+svd()
}
robust_inverse ..> np.linalg : uses
Class diagram for CBMREstimator changes with robust_inverse integrationclassDiagram
class CBMREstimator {
_glh_con_group()
_glh_con_moderator()
}
class robust_inverse {
+robust_inverse(FI: np.ndarray, eps: float = 1e-8) np.ndarray
}
CBMREstimator ..> robust_inverse : uses in _glh_con_group, _glh_con_moderator
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @yifan0330 - I've reviewed your changes - here's some feedback:
- In
robust_inverse
, you’re only zeroing out small singular vectors (viaU * M
), but you still invert all singular values inS_inv
; you should maskS_inv
too (e.g.S_inv = np.where(S > eps, 1/S, 0)
) to avoid amplifying tiny values. - Consider adding targeted unit tests for
robust_inverse
(e.g., on singular or ill-conditioned matrices) to ensure it behaves as expected and thresholds values properly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `robust_inverse`, you’re only zeroing out small singular vectors (via `U * M`), but you still invert all singular values in `S_inv`; you should mask `S_inv` too (e.g. `S_inv = np.where(S > eps, 1/S, 0)`) to avoid amplifying tiny values.
- Consider adding targeted unit tests for `robust_inverse` (e.g., on singular or ill-conditioned matrices) to ensure it behaves as expected and thresholds values properly.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nimare/utils.py
Outdated
FI_inv = U @ np.diag(S_inv) @ U.T | ||
return FI_inv |
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.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable
)
FI_inv = U @ np.diag(S_inv) @ U.T | |
return FI_inv | |
return U @ np.diag(S_inv) @ U.T |
Closes # .
Changes proposed in this pull request:
Summary by Sourcery
Add a robust inverse function for Fisher information matrices using SVD thresholding and integrate it into CBMR inference to replace direct matrix inversion for enhanced numerical stability, and update CBMR tests accordingly.
New Features:
Enhancements:
Documentation:
Tests: