Fix test cases and improve MATLAB parity

[arnodelorme]

Summary

• Fix ICA randomization to match MATLAB's RNG behavior
• Remove compare ICAs functionality
• Add parity tests for ICL feature extractor
• Various test case fixes and improvements across the test suite

Changes include

• ICA/runica parity improvements with MATLAB
• Test case fixes for numerical comparisons
• Stage comparison utilities for pipeline debugging
• Improved channel handling and checkset validation

Test plan

• Run full test suite: python -m unittest discover -s tests
• Verify MATLAB parity tests pass (requires MATLAB engine)
• Check bids_preproc pipeline on sample datasets

🤖 Generated with Claude Code

[claude]

Pull Request Review: Fix test cases and improve MATLAB parity

Executive Summary

This PR makes significant progress toward MATLAB-Python parity for EEG processing, primarily by implementing a full runica (ICA) algorithm in Python and fixing numerous test cases. The changes span 65 files with 6,846 additions and 802 deletions.

---

Strengths

1. Comprehensive ICA Implementation

• The new runica.py (1,379 lines) is a well-documented Python translation of EEGLAB's runica function
• Achieves 99.97-99.99% component-wise correlation with MATLAB
• Includes extensive docstrings explaining parameters and algorithm behavior

2. Robust Test Coverage

• Added comprehensive test suites: test_runica.py (699 lines), test_parity_rng.py (331 lines), test_eeg_checkset.py (641 lines)
• Tests clearly separate MATLAB-dependent from standalone tests

3. Better Error Handling

• New strict_mode context manager in eeg_checkset.py provides flexible error handling
• Allows tests to handle exceptions gracefully while maintaining strict validation in production

4. Pipeline Debugging Tools

• New stage_comparison.py utility (579 lines) enables systematic MATLAB-Python comparison
• Includes ICA component matching by scalp map correlation

---

Issues Requiring Attention

1. Binary Data Files in Repository (HIGH PRIORITY)

Severity: High - Repository Size Impact

The PR adds ~25MB of binary test data files to the repository. This will bloat the git history permanently.

Recommendations:

• Use Git LFS for binary files
• Host test data externally (Zenodo, OSF, or institutional repository)
• Download test data on-demand during test setup
• Add these files to .gitignore and provide a setup script

2. Incomplete Error Handling in Channel Selection

Severity: Medium - In bids_preproc.py:573-584

The channel type handling has redundant float/nan checks that could be simplified.

3. Potential Index Confusion in Run Filtering

Severity: Medium - In bids_list_eeg_files.py:85-100

Creates inconsistent API where run=1 means run-1 but subject=1 means second subject alphabetically. Should document this clearly.

4. Missing Validation in ICA Functions

Severity: Low - In pop_subcomp.py and eeg_runica.py

Missing validation for ICA matrix shapes could lead to confusing errors.

5. Hardcoded Paths and Configuration

Severity: Low - In bids_preproc_accuracy_report.py:15-53

Host-specific paths are hardcoded. Consider using environment variables or config files.

6. Parameter Naming Ambiguity

Severity: Low - In bids_preproc.py

Both WithICA and WithPicard parameters interact. Consider deprecating WithPicard in favor of ICAAlgorithm parameter.

---

Code Quality & Best Practices

Positive Patterns

• Good type hints in some modules
• Comprehensive documentation in runica.py and test files
• Appropriate use of logging throughout
• Good use of context managers for mode switching

Areas for Improvement

• Inconsistent type hints across codebase
• bids_preproc function is very long - consider extracting helpers
• Some magic numbers could be constants (though most already are)

---

Security Considerations

Good Practices

• Proper use of tempfile module
• Appropriate path handling with os.path.join
• No dangerous shell execution

Recommendations

• Fix file permissions: Binary files have executable permissions (-rwxr-xr-x), should be -rw-r--r--
• Consider adding path validation to prevent traversal attacks

---

Performance Considerations

Optimization Opportunities

• ICA component matching uses O(n²) greedy algorithm - could use Hungarian algorithm (scipy.optimize.linear_sum_assignment)
• Check if data copies in eeg_runica.py are necessary
• ICA activations stored in full resolution - consider lazy computation for large datasets

Good Patterns

• Efficient NumPy vectorization throughout
• Conditional computation of ICA activations

---

Test Coverage Assessment

Excellent Coverage

• Core ICA functionality with extensive tests
• Multiple MATLAB parity test files
• Edge cases covered in test_eeg_checkset.py
• RNG behavior documented in test_parity_rng.py

Gaps

• Limited end-to-end integration tests
• Few tests for exception handling
• No performance benchmarks
• No doctests for documentation examples

---

Recommendations Summary

Must Fix Before Merge

1. Address binary data files - use LFS or external hosting
2. Fix redundant type checking in get_chan_type()
3. Document run vs subject index behavior clearly

Should Fix Before Merge

4. Add validation for ICA matrix shapes
5. Fix file permissions on binary files
6. Consider API deprecation path for WithPicard

Nice to Have

7. Add type hints consistently
8. Extract helper functions from long bids_preproc()
9. Use Hungarian algorithm for component matching
10. Add integration and property-based tests

---

Conclusion

This PR represents substantial progress toward MATLAB-Python parity. The ICA implementation is well-crafted, and the test coverage is impressive. However, the binary data files issue must be addressed before merging to avoid repository bloat.

Recommendation: Request changes to address the binary files issue.

Great work on the MATLAB parity improvements!

[claude]

PR Review: Fix test cases and improve MATLAB parity (#37)

Thank you for this comprehensive PR! This is a significant effort to improve test coverage and MATLAB parity for the eegprep library. Below is a detailed review covering code quality, potential issues, performance, security, and test coverage.

Summary

This PR adds 6,853 lines and removes 804 lines across 67 files, primarily focusing on:

• Implementation of runica.py (1,379 lines) - a complete Python port of MATLAB's ICA algorithm
• Enhanced eeg_checkset.py with strict mode and ICA RMS scaling
• Improved test coverage with extensive parity tests
• Binary test data files (.fdt, .set)
• Various bug fixes in preprocessing pipeline

Code Quality & Best Practices ✅

Strengths:

1. Excellent documentation - The runica.py file has outstanding docstrings explaining MATLAB parity, scientific validation, and algorithm details
2. Consistent logging - Good use of Python's logging module throughout (converted from print statements)
3. Type hints improving - Better use of type annotations in newer code
4. Context managers - Good use of strict_mode() context manager in eeg_checkset.py
5. Code organization - Clean separation of concerns with helper functions

Areas for Improvement:

1. Inconsistent error handling in pop_reref.py

# Line 37: Assumes icachansind exists
if len(EEG['icachansind']) and (len(EEG['icachansind']) != EEG['nbchan']):

Issue: This will raise KeyError if icachansind doesn't exist. Should use .get():

if EEG.get('icachansind') and len(EEG['icachansind']) != EEG['nbchan']:

2. Code duplication in channel validation

In pop_select.py (lines 137-149), there's duplicated warning logic for channel not found. Consider extracting to a helper function.

3. Magic numbers without constants

# eeg_checkset.py:120
if mean_diff < 0.0001:

Consider defining as a named constant: ICA_VALIDATION_THRESHOLD = 1e-4

4. Commented-out code in pop_reref.py

Lines 53-56 have commented code that should either be removed or documented why it's kept for reference.

Potential Bugs 🐛

High Priority:

1. Race condition in pop_select.py:37

if len(EEG['icachansind']) and (len(EEG['icachansind']) != EEG['nbchan']):

If icachansind is missing or None, this will crash. Needs defensive check.

2. Division by zero risk in eeg_runica.py:49

variance_metric = np.sum(EEG['icawinv'] ** 2, axis=0) * np.sum(icaact_2d ** 2, axis=1)

If all activations are zero, this could produce NaN values. Add validation.

3. Array shape assumptions

Multiple locations assume 3D data shapes without validation:

• eeg_runica.py:41 - assumes reshape will work
• pop_select.py:395 - assumes data.ndim works as expected

Medium Priority:

4. Loop variable reuse in pop_select.py

Lines 431-447 have complex nested logic for channel index mapping that could be simplified and made more robust with numpy operations.

Performance Considerations ⚡

Good:

1. Efficient numpy operations - Good use of broadcasting and vectorization
2. Memory-aware design - MaxMem parameter in bids_preproc.py
3. Parallel processing support - NumJobs parameter for multi-processing

Concerns:

1. Large binary files in repository (~2MB total)

data/eeglab_data.fdt
data/eeglab_data_epochs_ica.fdt
data/eeglab_data_hdf5.set

Recommendation: Consider using Git LFS for binary test fixtures or host them externally. This will bloat the repository over time.

2. Deep copying overhead

pop_reref.py:27 and pop_select.py:27 use deepcopy(EEG) which can be expensive for large datasets. Consider:

• Documenting that functions modify in-place vs return copies
• Using selective copying only for modified fields
• Adding a copy parameter to let users opt-in

3. Repeated pseudoinverse computations

Multiple files compute np.linalg.pinv() which is O(n³). Consider caching when possible or computing once and reusing.

Security Concerns 🔒

Low Risk:

1. File path handling - eeg_checkset.py:99-104 constructs file paths from user data. Good use of os.sep and validation.
2. No SQL injection risks - No database operations
3. No obvious code injection - No eval/exec usage

Recommendations:

1. Path traversal protection - Add validation in pop_loadset.py to prevent ../../ attacks
2. Input validation - Add bounds checking for numerical parameters in runica.py

Test Coverage 📊

Excellent additions:

1. ✅ Comprehensive parity tests: test_runica.py, test_ICL_feature_extractor_parity.py, test_parity_rng.py
2. ✅ New test file: test_eeg_checkset.py with 641 lines covering validation logic
3. ✅ Integration tests: test_pipeline.py improvements
4. ✅ Edge case coverage: Tests for empty arrays, boundary conditions

Coverage gaps:

1. ❌ Error path testing - Not enough tests for exception handling
2. ❌ Performance tests - No tests for large dataset handling
3. ⚠️ Mock MATLAB engine - Tests requiring MATLAB will be skipped in CI without the engine

Specific test concerns:

1. Skipped tests without MATLAB

test_matlab_path.py:36 - Tests gracefully skip when MATLAB CLI unavailable, but this means some validation only happens in specific environments.

2. Test data dependencies

Many tests depend on specific binary files. Ensure these are documented and version-controlled properly.

Additional Observations

CI/CD:

• ✅ Good: MATLAB batch licensing setup in .github/workflows/test.yml
• ⚠️ Concern: Secret name changed from MLM_LICENSE_TOKEN to MATLAB_BATCH_TOKEN - ensure this is updated in GitHub Actions secrets

Documentation:

• The PR description is clear and well-structured
• Commit messages could be more descriptive (many are very terse like "fix tests")
• Consider adding migration notes if APIs changed

Recommendations Summary

Must Fix (P0):

1. Add defensive checks for icachansind in pop_reref.py
2. Validate array shapes before reshape operations
3. Update GitHub Actions secret name or revert to original

Should Fix (P1):

1. Remove commented code or document why it's kept
2. Extract magic numbers to named constants
3. Consider Git LFS for binary test data
4. Add error path tests

Nice to Have (P2):

1. Reduce deep copy overhead with selective copying
2. Document performance characteristics
3. Add type hints to remaining functions
4. Consolidate duplicate validation logic

Conclusion

This is high-quality work with excellent MATLAB parity testing and comprehensive coverage. The implementation of runica.py is particularly impressive with detailed validation and documentation. The main concerns are around defensive programming for edge cases and repository size due to binary files.

Recommendation: ✅ Approve with minor fixes

The issues identified are mostly minor and can be addressed in follow-up commits or a quick revision. The benefits of improved test coverage and MATLAB parity far outweigh the concerns.

Great work on this substantial contribution! 🎉

[claude]

PR #37 Code Review: Fix test cases and improve MATLAB parity

This is a large, complex PR (6863 additions, 814 deletions, 66 files) that makes substantial improvements to ICA functionality and test infrastructure. Code quality is generally high, with good documentation and extensive testing.

HIGH-PRIORITY ISSUES

1. Temporary File Cleanup (stage_comparison.py:104-145)

Temporary files are not properly cleaned up in error cases. Use try-finally blocks with contextlib.suppress(FileNotFoundError).

2. Missing Input Validation (runica.py:218-226)

Add validation when converting input data to float64 and check for 2D array shape.

3. Integer Overflow Risk (ransac.py:46)

Add bounds checking when casting to int in rand_sample function.

MEDIUM-PRIORITY ISSUES

• Verbose logging in runica.py - use logger.debug() instead of logger.info() for detailed progress
• Code duplication in training loops (runica.py:683-1291) - refactor to single loop with conditionals
• Unbounded array growth - pre-allocate arrays instead of using append() in loops
• Print statements instead of logging (eeg_checkset.py:318-361)
• Typo: "dictionnary" should be "dictionary" (eeg_checkset.py:318)

PERFORMANCE CONCERNS

• Cache repeated matrix multiplications (weights @ sphere computed multiple times)
• O(n²) correlation matrix could be vectorized

TEST COVERAGE

Excellent test structure with comprehensive parity tests. Potential gaps:

• No tests for stage_comparison.py error paths
• Limited tests for pop_subcomp.py edge cases
• No tests for memory limits in large-scale ICA

FINAL VERDICT

Recommendation: APPROVE WITH CHANGES

Key Strengths:

• Comprehensive test coverage with MATLAB parity validation
• Well-documented algorithms with mathematical references
• No critical security vulnerabilities

Required Changes:

• Fix 3 HIGH-priority issues (temp file cleanup, input validation, integer overflow)
• Address MEDIUM-priority logging and error handling issues

Code Quality Score: 8.5/10

The PR is production-ready after addressing the high-priority issues. Medium/low-priority issues can be addressed in follow-up PRs.

[claude]

PR Review: Fix test cases and improve MATLAB parity

Thank you for this comprehensive PR! This is a substantial effort to improve MATLAB-Python parity for ICA decomposition. I've conducted a thorough review and have feedback organized by category.

---

🎯 Overall Assessment

Strengths:

• Well-documented implementation with clear references to MATLAB sources
• Comprehensive test coverage including parity tests
• Good separation of concerns (runica.py, test files, utilities)
• Thoughtful handling of RNG compatibility issues

Areas for Improvement:

• Binary data files committed to repo (see below)
• CI workflow changes need clarification
• Some validation and error handling opportunities

---

📊 Code Quality & Best Practices

✅ Excellent Practices

1. Documentation (src/eegprep/runica.py:1-19): Excellent docstring explaining the MATLAB derivation, parity achievements (99.97-99.99%), and scientific references.

2. Constants (src/eegprep/runica.py:30-57): Well-organized constants matching MATLAB defaults with descriptive names.

3. Test Organization (tests/test_runica.py, tests/test_parity_rng.py): Clear separation between functionality tests and parity tests, with excellent comments explaining RNG behavior.

4. Error Context (src/eegprep/eeg_checkset.py:20-42): The strict_mode context manager is a clean pattern for controlling error handling behavior.

🔍 Code Quality Observations

1. Parameter Validation (src/eegprep/runica.py:300-364): Good validation of string parameters, but consider consolidating the multiple if 'param' in kwargs_lower blocks into a single parameter processing function for maintainability.

2. Type Hints: The codebase would benefit from more comprehensive type hints. For example:

   • runica.py:60: Function signature could use type hints for parameters
   • eeg_checkset.py:45: Missing type hints for EEG dict and return value

3. Magic Numbers (src/eegprep/utils/ransac.py:45): The round_mat usage is well-explained in comments, but ensure this MATLAB compatibility pattern is documented in a central location.

---

🐛 Potential Bugs & Issues

⚠️ Critical Issues

1. Binary Data Files Committed (.gitignore:66-77, 96-98):

   • The PR adds 6 binary data files (*.set, *.fdt) to the data/ directory
   • These files are listed in .gitignore (lines 66-77, 96-98) but were still committed
   • This will bloat the repository permanently
   • Recommendation: Remove these files from git history using git rm --cached and rely on test fixtures generated at runtime or stored via Git LFS

2. CI Workflow Matrix Reduction (.github/workflows/test.yml:25):

   • Removed Python 3.13, macOS, and Windows testing
   • Question: Is this intentional? Removing cross-platform and latest Python version testing reduces coverage
   • If this is temporary due to MATLAB licensing, please add a TODO comment explaining the plan to restore full coverage

3. MATLAB Skip Flag (.github/workflows/test.yml:54):

   • Line 54 unconditionally sets EEGPREP_SKIP_MATLAB=1 even when MATLAB is available
   • This appears to disable MATLAB parity tests in CI
   • Recommendation: This should be inside the else block (lines 50-52) or removed if MATLAB tests should run

⚠️ Moderate Issues

1. Missing Validation (src/eegprep/pop_subcomp.py:59):

   • components = np.array(components, dtype=int) doesn't validate that indices are within valid range
   • Recommendation: Add bounds checking to prevent out-of-range component indices

2. Error Handling (src/eegprep/eeg_checkset.py:96-99):

   • File loading uses string path concatenation with os.sep
   • Recommendation: Use os.path.join() for cross-platform compatibility

3. Potential Memory Issue (src/eegprep/pop_subcomp.py:82-84):

   • Reshaping and matrix operations on EEG['data'] could be memory-intensive for large datasets
   • Consider adding a check for data size or processing in chunks for very large datasets

---

⚡ Performance Considerations

✅ Good Optimizations

1. Fisher-Yates Shuffle (src/eegprep/utils/ransac.py:10-49): Excellent optimization from O(n²) to O(n), with clear documentation of the 25x speedup.

2. Component Matching (src/eegprep/utils/stage_comparison.py:9-41): The greedy matching algorithm is reasonable for component reordering. For very large component counts, consider optimizing with Hungarian algorithm if needed.

💡 Performance Suggestions

1. Matrix Operations (src/eegprep/pop_subcomp.py:73-84):

   • Multiple matrix multiplications: W = icaweights @ icasphere, then pinv(W), then A_clean @ W @ data_2d
   • Consider caching intermediate results if this function is called multiple times

2. Correlation Matrix (src/eegprep/utils/stage_comparison.py:19-22):

   • Nested loop computing all pairwise correlations
   • For large component counts, this could be vectorized using NumPy operations

---

🔒 Security Concerns

✅ No Major Security Issues

The code appears safe from common vulnerabilities:

• No SQL injection risks
• No command injection (Bash calls are in test setup, not user-controlled)
• No XSS risks (scientific computing, not web)
• File operations use safe patterns

💡 Minor Security Observations

1. Temp File Handling (tests/test_parity_rng.py:49, 85):

   • Uses tempfile.mktemp() which is deprecated due to race conditions
   • Recommendation: Use tempfile.NamedTemporaryFile() or tempfile.mkstemp()

2. File Path Handling (src/eegprep/eeg_checkset.py:98):

   • Concatenates paths without validation
   • Recommendation: Add path validation to ensure files are within expected directories

---

🧪 Test Coverage

✅ Excellent Test Coverage

1. New Test Files:

   • test_runica.py: Comprehensive functionality tests (699 lines)
   • test_parity_rng.py: RNG compatibility tests (331 lines)
   • test_eeg_checkset.py: Validation tests (641 lines)
   • test_ICL_feature_extractor_parity.py: Feature extractor parity (211 lines)

2. Test Quality: Tests include both unit tests and integration/parity tests with MATLAB

3. Edge Cases: Tests cover various parameter combinations, error conditions, and boundary cases

💡 Test Coverage Suggestions

1. Component Removal (src/eegprep/pop_subcomp.py):

   • Add tests for edge cases: removing all components, removing no components, invalid indices
   • Test memory efficiency with large datasets

2. Error Paths (src/eegprep/eeg_checkset.py):

   • More tests for the strict mode vs non-strict mode error handling
   • Tests for malformed input data

3. Integration Tests:

   • Consider adding end-to-end pipeline tests that combine multiple stages
   • Test the full bids_preproc workflow with the new runica implementation

---

📝 Specific File Recommendations

src/eegprep/runica.py

• Line 300-364: Consider extracting parameter parsing into a separate function
• Line 396-399: Good validation, but consider adding helpful error messages with current values
• Add type hints to function signature

src/eegprep/eeg_checkset.py

• Line 96-99: Use os.path.join() instead of string concatenation
• Line 151+: Add docstrings for any additional validation logic

src/eegprep/pop_subcomp.py

• Line 59: Add bounds checking for component indices
• Line 73-84: Consider adding progress logging for large datasets
• Add type hints

tests/test_parity_rng.py

• Line 49, 85: Replace tempfile.mktemp() with tempfile.NamedTemporaryFile(delete=False)

.github/workflows/test.yml

• Line 25: Document why Python 3.13, macOS, and Windows were removed
• Line 54: Fix MATLAB skip flag logic
• Consider adding a comment explaining the MATLAB licensing situation

pyproject.toml

• Line 77: Good addition of resources/eeglab/**/* to package data

---

🎓 Summary & Recommendations

Priority Actions

1. HIGH: Remove binary data files from git and update .gitignore handling
2. HIGH: Fix CI workflow MATLAB skip flag logic (line 54)
3. MEDIUM: Document CI test matrix reduction or restore full coverage
4. MEDIUM: Add bounds checking for component indices in pop_subcomp
5. MEDIUM: Replace deprecated tempfile.mktemp() calls
6. LOW: Add type hints to major functions
7. LOW: Consolidate parameter parsing in runica.py

Merge Recommendation

This PR represents significant work improving MATLAB parity and is well-tested. The binary data file issue is the main blocker. Once addressed, this should be ready to merge with the minor improvements noted above addressed in follow-up PRs if needed.

Suggested approach:

1. Address HIGH priority items before merging
2. Create issues for MEDIUM priority items to track post-merge
3. Consider LOW priority items for future refactoring

---

Great work on achieving 99.97-99.99% MATLAB parity! The RNG compatibility documentation and Fisher-Yates optimization are particularly impressive. 🚀

[claude]

Pull Request Review: #37 - Fix test cases and improve MATLAB parity

Thank you for this substantial contribution! This PR adds significant functionality with the runica implementation and improves MATLAB parity. I've conducted a comprehensive review and found the code to be generally well-structured, but there are several critical issues that need attention before merging.

---

🔴 CRITICAL ISSUES

1. API Mismatch in stage_comparison.py (Lines 50, 170, 229, 271)

Problem: The code imports and calls a non-existent function:

from eegprep import pop_loadset, eeg_checkset_strict_mode
with eeg_checkset_strict_mode(False):

Fix: Should be:

from eegprep.eeg_checkset import strict_mode
with strict_mode(False):

Impact: This module is completely non-functional. All comparison utilities will fail on import/runtime.

---

2. Confusing Strict Mode Logic in eeg_checkset.py (Lines 52-55)

Problem: The exception type assignment is semantically inverted:

exception_type = DummyException if _strict_mode_var.get() else Exception

This works correctly by accident but is extremely confusing. The naming suggests the opposite behavior.

Recommendation: Either:

• Invert the variable semantics and rename to _permissive_mode_var
• Add extensive documentation explaining why DummyException means "strict"
• Refactor to be more explicit about intent

Impact: Future maintainers will likely introduce bugs when modifying this code.

---

3. Shape Handling Bug in pop_reref.py (Line 34)

Problem: Mean subtraction doesn't preserve dimensions for 3D epoched data:

EEG['data'] = EEG['data'] - np.mean(EEG['data'], axis=0)

Fix:

EEG['data'] = EEG['data'] - np.mean(EEG['data'], axis=0, keepdims=True)

Impact: Will crash on epoched data with broadcasting errors.

---

4. ICA Sphere Matrix Corruption in pop_reref.py (Line 51)

Problem: Setting icasphere = np.eye(EEG['nbchan']) breaks the mathematical relationship between weights and sphere:

EEG['icasphere'] = np.eye(EEG['nbchan'])

Impact: ICA decomposition consistency is violated. The identity sphere means weights are no longer properly whitened.

---

🟡 HIGH PRIORITY ISSUES

5. Potential Infinite Loop in runica.py (Lines 725-729)

Problem: RNG for kurtosis sampling can produce zeros indefinitely if datalength=1:

rp = (rng.rand(kurtsize) * datalength).astype(int)
while np.any(rp == 0):
    ou = np.where(rp == 0)[0]
    rp[ou] = (rng.rand(len(ou)) * datalength).astype(int)

Additionally, this biases the distribution (zero is never selected).

Fix: Use rp = rng.integers(1, datalength + 1, size=kurtsize) (1-indexed matching MATLAB) and avoid the while loop.

---

6. Suboptimal Component Matching in stage_comparison.py (Lines 30-40)

Problem: Uses greedy matching instead of optimal assignment:

for mat_idx in range(n_comps):
    # Find best unused Python component
    best_py_idx = -1
    best_corr = -1
    for py_idx in range(n_comps):
        if py_idx not in used and corr_matrix[py_idx, mat_idx] > best_corr:

Impact: May fail to find globally maximum correlation assignment. Could match components incorrectly.

Fix: Use scipy.optimize.linear_sum_assignment (Hungarian algorithm).

---

7. Sign Information Loss in Component Matching (Line 23)

Problem: Taking absolute value of correlation removes phase information:

corr_matrix[i, j] = abs(np.corrcoef(icawinv_py[:, i], icawinv_mat[:, j])[0, 1])

Impact: Phase-flipped components (negative correlation) are treated identically to aligned components. This is a loss of diagnostic information.

---

8. Performance Issues in runica.py

Line 480-481: Loop-based mean removal:

for i in range(data.shape[0]):
    data[i, :] = data[i, :] - rowmeans[i]

Should be: data = data - rowmeans[:, np.newaxis]

Lines 687, 874, 1018, 1179: custom_randperm() called at every training step. For 512 steps with 30K timepoints, this is expensive. Consider caching or optimizing.

---

🟢 MEDIUM PRIORITY ISSUES

9. Validation Gaps

• eeg_checkset.py:152: icachansind not validated for duplicates/sorted order
• eeg_checkset.py:120: Hardcoded threshold 0.0001 for RMS scaling - needs documentation
• runica.py:746: No validation that signs matrix contains only ±1 (could be zeros if kurtosis ≈ -signsbias)
• bids_list_eeg_files.py:50: Filter removal with if v rejects valid falsy values (e.g., subjects=0)

10. Numerical Stability Concerns

• pop_subcomp.py:74-75: Computing W = icaweights @ icasphere then pinv(W) compounds errors. Better to work with icaweights/icasphere separately.
• pop_subcomp.py:89-90: No validation that pinv(new_icaweights @ icasphere) == new_icawinv after component removal
• eeg_checkset.py:168: Hard-coded float32 conversion loses precision unnecessarily

---

📊 TEST COVERAGE GAPS

Missing Tests:

• runica.py:

  • Weight blowup and restart mechanism
  • Momentum + extended-ICA combination
  • Edge cases: lrate=0, ncomps < chans, data with NaN/Inf

• eeg_checkset.py:

  • strict_mode(False) with actual exception triggering
  • ICA RMS scaling with zero-variance components
  • Epoch structure with non-standard shapes

• stage_comparison.py:

  • Different numbers of components between implementations
  • Greedy matching against known ground truth
  • Graceful degradation without MATLAB

• pop_subcomp.py:

  • Component removal with 3D epoched data
  • Reconstruction error validation
  • All components removed (edge case)

• pop_reref.py:

  • Re-referencing with 3D epoched data
  • ICA consistency after re-reference

---

✅ STRENGTHS

1. Excellent documentation in runica.py with MATLAB line references and parity reports
2. Comprehensive parameter validation in runica implementation
3. Well-organized test suite with 699 lines of runica tests
4. Proper use of context managers for strict mode (despite naming confusion)
5. Numerical stability considerations (clipping, float64 conversion, sqrtm handling)
6. Modular design with clear separation of concerns

---

🔧 RECOMMENDATIONS

Immediate Actions (Blockers):

1. Fix stage_comparison.py API calls
2. Fix pop_reref.py shape handling and sphere matrix
3. Add validation to prevent critical edge cases in runica.py
4. Clarify/refactor strict_mode implementation

Before Merge:

1. Add tests for all identified edge cases
2. Document hardcoded thresholds (0.9, 0.0001, 1e8)
3. Consider Hungarian algorithm for component matching
4. Optimize performance bottlenecks in runica

Post-Merge:

1. Profile runica performance vs MATLAB
2. Add integration tests for full pipeline stages
3. Document RNG parity decisions for future reference

---

📈 OVERALL ASSESSMENT

Code Quality: 7/10 - Well-structured but several critical bugs
Test Coverage: 6/10 - Good baseline but missing edge cases
Documentation: 9/10 - Excellent inline documentation
Performance: 6/10 - Some inefficiencies identified
Security: 9/10 - No security concerns identified

Recommendation: ⚠️ REQUEST CHANGES - Critical bugs must be fixed before merge. The foundation is solid, but the identified issues will cause production failures.

---

Let me know if you'd like me to clarify any of these findings or help with fixes!

[claude]

PR #37 Code Review: ICA Randomization Fixes and MATLAB Parity Improvements

This is a substantial and well-implemented PR (6,885 additions, 67 files) that significantly improves ICA reproducibility and MATLAB compatibility. Overall quality is high with excellent documentation and testing. However, I've identified several issues requiring attention.

---

🚨 Critical Issues (Must Fix)

1. Strict Mode Logic Inverted in eeg_checkset.py:55

Location: src/eegprep/eeg_checkset.py:55

The strict mode exception handling is backwards:

exception_type = DummyException if _strict_mode_var.get() else Exception

Since DummyException is never raised (line 22), this means:

• When strict_mode(True) (should be strict): catches DummyException → catches nothing → errors are silently swallowed ❌
• When strict_mode(False) (should be lenient): catches Exception → catches everything → errors propagate ❌

Fix:

exception_type = Exception if _strict_mode_var.get() else DummyException

Impact: Affects error handling in lines 138-139, 169-170, 252-253. Currently fails silently when it should raise.

2. Print Statements Instead of Logger

Location: src/eegprep/eeg_checkset.py:318, 340, 345, 348, 361

Uses print() instead of logger.warning(). This breaks logging consistency and makes it difficult to control output in production.

Fix: Replace all print() calls with appropriate logger calls:

logger.warning(f"Warning message here")

---

⚠️ High Priority Issues (Should Fix)

3. Unnecessary Zero Index Handling in runica.py

Location: src/eegprep/runica.py:725-729, 1049-1052

rp = (rng.rand(kurtsize) * datalength).astype(int)
while np.any(rp == 0):
    ou = np.where(rp == 0)[0]
    rp[ou] = (rng.rand(len(ou)) * datalength).astype(int)

This checks for index 0 and regenerates it. In MATLAB (1-based indexing), 0 means "invalid". In Python (0-based), 0 is a valid index. This is legacy code from direct MATLAB translation.

Recommendation: Remove the while loop entirely. The indices 0 to datalength-1 are all valid in Python.

4. Type Hint Error in stage_comparison.py

Location: src/eegprep/utils/stage_comparison.py:45, 158, etc.

def compare_ica_decompositions(...) -> Dict[str, any]:

Should be Any (capital A) from the typing module, not lowercase any.

Fix:

from typing import Dict, Any
def compare_ica_decompositions(...) -> Dict[str, Any]:

5. Missing Temp File Cleanup

Location: src/eegprep/utils/stage_comparison.py:112-139

temp_py = tempfile.NamedTemporaryFile(suffix='_py.set', delete=False)

If generate_ica_comparison_plot() raises an exception before cleanup, temp files persist.

Fix: Use try-finally or context managers for guaranteed cleanup.

---

💡 Medium Priority Issues (Consider Fixing)

6. Angle Delta Clipping Could Mask Issues

Location: src/eegprep/runica.py:832-835 (and similar in other training loops)

cos_angle = (delta @ olddelta) / np.sqrt(change * oldchange)
cos_angle = np.clip(cos_angle, -1.0, 1.0)  # Clip to [-1, 1] for acos
angledelta = np.arccos(cos_angle)

While clipping prevents NaN, it silently masks numerical instability. If cosine is > 1.0 due to floating-point errors, clipping changes the value without warning.

Recommendation: Add a tolerance check before clipping:

if np.abs(cos_angle) > 1.0 + 1e-10:
    logger.warning(f"Cosine value {cos_angle:.2e} exceeds valid range, clipping applied")
cos_angle = np.clip(cos_angle, -1.0, 1.0)

7. Component Matching Algorithm Suboptimal

Location: src/eegprep/utils/stage_comparison.py:25-40

The greedy matching algorithm is O(n²) and doesn't guarantee globally optimal matching.

Recommendation: Use Hungarian algorithm for optimal matching:

from scipy.optimize import linear_sum_assignment
cost_matrix = 1 - corr_matrix  # Convert similarity to cost
row_ind, col_ind = linear_sum_assignment(cost_matrix)

8. Data Loading Path Construction

Location: src/eegprep/eeg_checkset.py:99-106

file_name = EEG['filepath'] + os.sep + EEG['data']

Should use os.path.join() for portability and assumes 'filepath' exists (no validation).

Fix:

file_name = os.path.join(EEG.get('filepath', ''), EEG['data'])

---

✅ Strengths

Excellent Implementation Quality

1. runica.py: Outstanding documentation (211 lines of docstring), comprehensive parameter handling, well-structured training loops, proper MATLAB parity mechanisms
2. RNG Parity: Clever use of rand_permutation() from utils/ransac.py to match MATLAB's randperm() - critical for reproducibility
3. Test Coverage: Comprehensive test suite with 30+ test methods in test_runica.py, proper parity testing framework
4. Documentation: Clear explanations of MATLAB parity mechanisms, well-documented known incompatibilities

Smart Design Choices

1. Strict Mode Context Manager: Uses Python's contextvars for thread-safe mode management
2. Hash-Based Versioning: Clever parameter hash for preprocessing variants in bids_preproc.py
3. Component Variance Sorting: Proper handling of non-square weight matrices with informative messages

---

📝 Test Coverage Assessment

Good Coverage

• test_runica.py: Excellent coverage of all ICA modes, parameters, edge cases
• test_eeg_checkset.py: Good basic structure validation
• test_parity_rng.py: Excellent documentation of RNG compatibility

Coverage Gaps

1. No tests for stage_comparison.py - Should test component matching, ICA comparison, ICLabel comparison
2. No tests for strict_mode behavior - Should verify error handling in both modes
3. Limited pipeline error recovery tests - Should test failure scenarios

---

🎯 Recommendations

Immediate Actions

1. Fix the strict_mode logic inversion in eeg_checkset.py:55
2. Replace print() statements with logger calls
3. Fix type hints (any → Any)

High Priority

1. Add comprehensive tests for stage_comparison.py
2. Remove unnecessary zero-index handling in runica.py
3. Improve temp file cleanup with proper exception handling

Quality Improvements

1. Add logging for angle delta clipping events
2. Consider Hungarian algorithm for optimal component matching
3. Use os.path.join() for cross-platform path construction
4. Add pipeline error recovery tests

---

📊 Overall Assessment

Rating: Strong Approve with Required Fixes ⭐⭐⭐⭐

This PR represents significant progress toward MATLAB parity and improved ICA reproducibility. The core algorithm implementation is excellent with proper documentation and comprehensive testing. The critical strict_mode bug must be fixed before merge, but otherwise this is high-quality work.

The 99.97-99.99% component-wise correlation with MATLAB demonstrates the quality of the implementation. Once the critical issues are addressed, this will be a solid foundation for EEG preprocessing pipelines.

---

Review completed with automated analysis. Please address the critical issues before merging.

[claude]

PR #37 Review: Fix test cases and improve MATLAB parity

Summary

This is a substantial PR with 67 changed files (+6899/-839 lines) that significantly improves test coverage, fixes numerous test cases, and enhances MATLAB parity for ICA operations. The changes span core preprocessing functions, test infrastructure, and CI/CD configuration.

Overall Assessment: ✅ Approve with Minor Recommendations

---

🎯 Strengths

1. Excellent Test Coverage

• Comprehensive test suite with clear parity testing strategy
• Well-documented test files (e.g., test_parity_rng.py:1-13 explains RNG mechanism)
• Proper use of fixtures and test organization
• Good separation between functionality tests and parity tests

2. Code Quality

• Clean implementation of runica.py with excellent documentation
• Proper use of logging instead of print statements (good fix in commit 53e5abb)
• Type conversions handled correctly (e.g., eeg_runica.py:27 forces float64)
• Constants clearly defined with MATLAB equivalents (runica.py:31-58)

3. MATLAB Parity Strategy

• Thoughtful approach to RNG compatibility documented in test_parity_rng.py
• Recognition that exact parity requires rand() not randn()
• Component correlation-based comparison (commit 681ae7a) is the right approach for ICA

---

🔍 Code Quality & Best Practices

Positive Observations

1. Proper Error Handling

# pop_reref.py:30-31 - Clear validation with helpful error message
if ref is not None and ref != []:
    raise ValueError('Feature not implemented: The ref parameter must be empty or None')

2. Clean Refactoring

• pop_reref.py:66 - Properly calls eeg_checkset() for data normalization
• eeg_runica.py:36 - Correct use of pseudoinverse via pinv(weights @ sphere)

3. Good Documentation

• Docstrings follow NumPy style consistently
• Parameter types and shapes clearly specified
• References to scientific papers included (runica.py:15-19)

Minor Concerns

1. Commented-out Code (pop_reref.py:54-56)

# data = EEG['data'].reshape(EEG['data'].shape[0], -1)
# EEG['icaact'] = np.dot(EEG['icaweights'], data)
# EEG['icaact'] = EEG['icaact'].reshape(EEG['icaweights'].shape[0], EEG['pnts'], EEG['trials'])

Recommendation: Remove commented code or add a TODO explaining why it's disabled.

2. Import Organization (pop_reref.py:7)

from eegprep.eeg_checkset import eeg_checkset

Should use relative import for consistency:

from .eeg_checkset import eeg_checkset

---

🐛 Potential Bugs & Issues

1. Array Shape Assumptions (eeg_runica.py:37-43)

if len(EEG['icachansind']) and (len(EEG['icachansind']) != EEG['nbchan']):

Issue: len() on numpy arrays is risky - should check size or shape[0]
Recommendation: Use explicit checks:

if EEG['icachansind'].size > 0 and EEG['icachansind'].size != EEG['nbchan']:

2. Division by Zero Risk (Potential in variance calculations)

In eeg_runica.py:49, variance calculations could produce zeros:

variance_metric = np.sum(EEG['icawinv'] ** 2, axis=0) * np.sum(icaact_2d ** 2, axis=1)

Recommendation: Add epsilon or check for zero variance components.

3. CI Configuration (.github/workflows/test.yml:25)

The matrix was reduced to only Python 3.12 on Ubuntu:

python-version: ["3.12"]

Question: Was this intentional? The PR removes macOS and Windows testing plus Python 3.10, 3.11, 3.13.
Recommendation: Restore cross-platform testing or document why it's removed.

---

⚡ Performance Considerations

1. Memory Efficiency

• eeg_runica.py:28 - Data is reshaped but original preserved (good for non-mutation)
• Could use in-place operations if memory is critical, but current approach is safer

2. Computational Efficiency

• eeg_runica.py:47 - Reshaping to 2D for variance is efficient
• Pseudoinverse computation (pop_reref.py:49) could be expensive for large channel counts
  • Consider caching if called repeatedly on same data

3. Test Performance

• Good use of maxsteps=10 in tests for speed (test_runica.py:32)
• MATLAB tests properly skipped when unavailable

---

🔒 Security Concerns

No Critical Security Issues Found ✅

Reviewed Areas:

• ✅ No command injection risks (proper parameter passing)
• ✅ No pickle/eval usage in this PR's changes
• ✅ File I/O uses safe methods (scipy.io, numpy)
• ✅ Input validation present where needed

Minor Note:
The PR touches MATLAB integration code, but doesn't modify the eeglabcompat.py file itself, so existing security considerations remain unchanged.

---

🧪 Test Coverage Assessment

Excellent Coverage ✅

New Test Files:

• test_runica.py (699 lines) - Comprehensive ICA testing
• test_parity_rng.py (331 lines) - RNG parity validation
• test_ICL_feature_extractor_parity.py (211 lines) - Feature extractor parity

Test Quality:

# test_runica.py:25-42 - Good test structure
def test_basic_ica(self):
    """Test basic ICA decomposition with default parameters."""
    np.random.seed(42)  # Reproducibility
    data = np.random.randn(10, 1000)
    
    weights, sphere, compvars, bias, signs, lrates = runica(...)
    
    # Check output shapes
    self.assertEqual(weights.shape, (10, 10))
    # Check all outputs are finite
    self.assertTrue(np.all(np.isfinite(weights)))

Strengths:

• Tests check both shape and data validity
• Proper use of assertions with meaningful messages
• Good test isolation and reproducibility

Coverage Gaps to Consider:

• Edge cases: What happens with single-channel data?
• What if EEG['icachansind'] is not an array but None?
• Boundary conditions for very small/large datasets

---

📋 Recommendations

High Priority

1. ✅ Restore CI Matrix - Clarify if removing macOS/Windows/other Python versions was intentional
2. 🔧 Fix Array Checks - Use .size instead of len() for numpy arrays
3. 🧹 Remove Dead Code - Clean up commented code in pop_reref.py

Medium Priority

4. 📝 Document CI Changes - Add comment explaining why test matrix was reduced
5. 🔍 Add Edge Case Tests - Test with minimal data (1-2 channels, few time points)
6. 🎯 Import Consistency - Use relative imports throughout

Low Priority (Nice to Have)

7. 📊 Add Coverage Reports - Integrate coverage.py to track test coverage percentage
8. 🚀 Performance Benchmarks - Add timing tests for runica on standard dataset sizes
9. 📚 Example Notebooks - Add example showing successful MATLAB parity

---

🎓 Educational Note: ICA Comparison Strategy

The correlation-based component matching (test_eeg_picard.py commit 681ae7a) is the correct approach because:

• ICA components are unique only up to permutation and sign
• Exact matrix comparison would fail even with identical decompositions
• Component correlation measures statistical equivalence

This is well-implemented! 👍

---

✅ Approval Status

This PR is ready to merge after addressing the CI configuration question. The code quality is high, test coverage is excellent, and the MATLAB parity improvements are well-designed.

Suggested merge strategy:

1. Confirm CI matrix reduction is intentional (or restore it)
2. Consider applying minor cleanup recommendations
3. Merge to develop for broader testing
4. Monitor for any edge cases in production use

---

📊 Statistics

• Files Changed: 67
• Additions: +6899 lines
• Deletions: -839 lines
• Test Files: 54+
• New Test Methods: 100+
• Commits: 84 (well-organized incremental progress)

Great work on improving test reliability and MATLAB parity! 🎉