Skip to content

Conversation

@edeno
Copy link
Contributor

@edeno edeno commented Sep 25, 2025

Summary

  • Replace direct matrix inverse with Tikhonov-regularized solve in MVAR Fourier coefficient computations
  • Add scale-aware regularization parameter: λ = 1e-12 * mean(||H||²)
  • Add stress test for near-singular frequency bins

Test plan

  • New stress test test_mvar_regularized_inverse_near_singular passes
  • All existing MVAR/Granger causality tests continue to pass
  • Directed connectivity measures (DTF, etc.) work with near-singular inputs
  • No LinAlgError exceptions thrown for highly correlated signal inputs

🤖 Generated with Claude Code

…R Fourier

- Replace xp.linalg.inv(H) with xp.linalg.solve(H + λI, I) in _MVAR_Fourier_coefficients
- Replace xp.linalg.inv(H_0) with regularized solve in _estimate_transfer_function
- Use scale-aware regularization: λ = 1e-12 * mean(||H||²) for stability
- Add stress test for near-singular frequency bins to verify no LinAlgError

This prevents numerical instability from singular or near-singular transfer
function matrices that can occur with highly correlated signals.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@edeno edeno requested a review from Copilot September 25, 2025 12:59
Copy link

Copilot AI left a 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 replaces direct matrix inversion operations in MVAR Fourier coefficient computations with Tikhonov-regularized linear solves to prevent numerical instability when dealing with near-singular frequency bins.

  • Replaces inv(H) with solve(H + λI, I) using scale-aware regularization
  • Adds comprehensive stress test for near-singular cross-spectral matrices
  • Applies regularization to both transfer function estimation and MVAR coefficient computation

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
spectral_connectivity/connectivity.py Implements Tikhonov regularization in _MVAR_Fourier_coefficients property and _estimate_transfer_function function
tests/test_connectivity.py Adds stress test test_mvar_regularized_inverse_near_singular for near-singular frequency bins

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 450 to 452
H = self._transfer_function
# Tikhonov regularization: solve(H + λI, I) instead of inv(H)
lam = 1e-12 * xp.mean(xp.real(xp.conj(H) * H)) # Scale-aware regularization
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regularization parameter lam uses a magic number 1e-12. Consider defining this as a named constant (e.g., TIKHONOV_REGULARIZATION_FACTOR = 1e-12) to improve maintainability and make it easier to adjust if needed.

Copilot uses AI. Check for mistakes.
)
H_0 = inverse_fourier_coefficients[..., 0:1, :, :]
# Tikhonov regularization: solve(H_0 + λI, I) instead of inv(H_0)
lam = 1e-12 * xp.mean(H_0 * H_0) # Scale-aware regularization for real matrix
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regularization parameter uses the same magic number 1e-12 as in the other function. Consider extracting this to a shared constant to ensure consistency and easier maintenance.

Copilot uses AI. Check for mistakes.
Comment on lines 618 to 619
base_signal = np.random.randn(n_time_samples, n_trials, n_tapers, n_fft_samples) + \
1j * np.random.randn(n_time_samples, n_trials, n_tapers, n_fft_samples)
Copy link

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The complex signal generation spans multiple lines with backslash continuation. Consider using parentheses for line continuation instead: base_signal = (np.random.randn(...) + 1j * np.random.randn(...))

Suggested change
base_signal = np.random.randn(n_time_samples, n_trials, n_tapers, n_fft_samples) + \
1j * np.random.randn(n_time_samples, n_trials, n_tapers, n_fft_samples)
base_signal = (
np.random.randn(n_time_samples, n_trials, n_tapers, n_fft_samples)
+ 1j * np.random.randn(n_time_samples, n_trials, n_tapers, n_fft_samples)
)

Copilot uses AI. Check for mistakes.
@coveralls
Copy link

coveralls commented Sep 25, 2025

Coverage Status

coverage: 76.304% (-0.2%) from 76.509%
when pulling 024dbc8 on feat/mvar-regularized-inverse
into 3e31c94 on master.

edeno and others added 5 commits September 25, 2025 09:01
- Break long lines in regularized inverse implementation
- Fix line length issues in new stress test
- Apply black formatting to ensure consistent style

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Address copilot review comments:
- Define TIKHONOV_REGULARIZATION_FACTOR = 1e-12 as module-level constant
- Use constant in both _MVAR_Fourier_coefficients and _estimate_transfer_function
- Improves maintainability and ensures consistency

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Merged latest master branch changes including new validation tests.
All tests from both branches are preserved.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Document Tikhonov regularization changes in PR #72 for numerical stability.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@edeno edeno merged commit a2b479e into master Oct 17, 2025
4 of 9 checks passed
@edeno edeno deleted the feat/mvar-regularized-inverse branch October 17, 2025 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants