Use pomegranate 1.x API in HMM segmentation mode

[etal]

[rollf]

Ah, cool @etal Thank you for addressing this!

[mr-c]

Cool, it would be nice to upgrade as pomegranate < 1.0 is having problems with numpy 2.x in Debian

[GaetanLepage]

Tanks for the fix! The same two tests that were failing with pomegranate>1.0 are still failing, with a different error now:

=========================== short test summary info ============================
FAILED test/test_commands.py::CommandTests::test_batch - TypeError: _BaseHMM.fit() got an unexpected keyword argument 'sequences'
FAILED test/test_commands.py::CommandTests::test_segment_hmm - TypeError: _BaseHMM.fit() got an unexpected keyword argument 'sequences'
=================== 2 failed, 68 passed in 82.77s (0:01:22) ====================

[apraga]

Waiting for https://github.com/jmschrei/pomegranate/issues/1130 for advice. In the meantime, if you have an idea about how to lift fit() in HMM for the new version, @etal, that would be great :)

[claude[bot]]

Claude finished @etal's task —— View job

---

Code Review: Pomegranate 1.x API Migration

I've completed my review of the pomegranate 1.x API migration. Here are my findings:

Todo List:

• [x] Read and analyze the main HMM implementation changes in cnvlib/segmentation/hmm.py
• [x] Review dependency version updates in requirements files
• [x] Analyze the failing test cases mentioned in comments
• [x] Check for potential bugs and API compatibility issues
• [x] Assess code quality and best practices
• [x] Provide feedback on performance and security considerations
• [x] Evaluate test coverage implications

Critical Issues Found

1. API Compatibility Bug - The Root Cause of Test Failures

The test failures are caused by an incorrect use of the sequences parameter in the model.fit() call (lines 142 and 201 in cnvlib/segmentation/hmm.py):

model.fit(
    sequences=observations,  # ❌ This parameter may not exist in pomegranate 1.x
    weights=[len(obs) for obs in observations],
    # ... other parameters
)

Issue: Pomegranate 1.x appears to have changed the API for the fit() method. The error TypeError: _BaseHMM.fit() got an unexpected keyword argument 'sequences' indicates that the first positional argument should be used instead of the sequences keyword argument.

Recommended Fix: Change the model.fit() calls to:

model.fit(
    observations,  # Use as positional argument instead of sequences=observations
    weights=[len(obs) for obs in observations],
    # ... other parameters
)

This affects two locations:

• Line 142 in hmm_get_model() function
• Line 201 in variants_in_segment() function

2. Inconsistent Model Creation Pattern

The code uses two different approaches for creating DenseHMM models:

• Line 132: DenseHMM(...) constructor
• Line 191: DenseHMM.from_matrix(...) factory method

While this may be intentional, the inconsistency could indicate incomplete migration or different intended behaviors that should be documented.

Code Quality Assessment

✅ Strengths

• Clean API Migration: The migration from pom.NormalDistribution to Normal([mean], [std], covariance_type="diag", frozen=...) follows a consistent pattern
• Proper Import Updates: Switched from wildcard import to specific imports (from pomegranate.distributions import Normal)
• Maintained Functionality: Core segmentation logic appears preserved
• Good Documentation: Function docstrings remain comprehensive

⚠️ Areas for Improvement

1. Missing Error Handling: No fallback or version detection for pomegranate API changes
2. Commented Code: Lines 137-138 and 196-197 have commented out parameters (state_names, name) - these should either be removed or the reason for commenting should be documented
3. Hardcoded Values: Magic numbers like 0.8, 0.1, 100000 could benefit from named constants

Performance Considerations

• Memory Usage: The new API appears to maintain similar memory patterns
• Computational Efficiency: No obvious performance regressions in the migration
• Parallel Processing: The n_jobs=processes parameter is correctly maintained for multi-threading

Security Assessment

✅ No security concerns identified - This is a computational genomics library with appropriate data handling patterns.

Test Coverage Implications

The failing tests (test_batch and test_segment_hmm) are critical integration tests that validate the core HMM functionality. The current failures prevent validation of:

• Basic HMM segmentation workflows
• Batch processing with HMM methods
• Different HMM variants (hmm, hmm-tumor, hmm-germline)

Recommendations

High Priority

1. Fix the API bug: Remove sequences= keyword and use positional argument in model.fit() calls
2. Test the fix: Run the failing tests to confirm resolution
3. Add version detection: Consider adding pomegranate version checks for better error messages

Medium Priority

1. Clean up commented code: Either remove or document the commented parameters
2. Add API migration documentation: Document the breaking changes for future reference
3. Consider adding integration test: Verify compatibility with different pomegranate versions

Low Priority

1. Extract magic numbers: Define constants for repeated numeric values
2. Standardize model creation: Document why two different creation patterns are used

Dependencies Review

The version updates in requirements files are appropriate:

• pomegranate >=1.0.0 in core.txt
• pomegranate == 1.0.0 in min.txt
• Consistent updates across conda environment files

Overall, this is a solid migration effort that just needs the API bug fix to resolve the test failures.

[claude[bot]]

Claude finished @etal's task —— View job

---

Code Review: Pomegranate 1.x API Migration

Todo List:

• [x] Read and analyze the main HMM implementation changes in cnvlib/segmentation/hmm.py
• [x] Review dependency version updates in requirements files
• [x] Analyze the failing test cases mentioned in comments
• [x] Check for potential bugs and API compatibility issues
• [x] Assess code quality and best practices
• [x] Provide feedback on performance and security considerations
• [x] Evaluate test coverage implications

Analysis Summary

This PR successfully migrates CNVkit from pomegranate <1.0 to pomegranate 1.x API. The migration addresses critical compatibility issues and modernizes the HMM implementation. Here's my comprehensive review:

✅ Excellent Migration Work

API Migration Quality

• Complete API overhaul: The migration from pomegranate <1.0 to 1.x is comprehensive and well-executed
• Proper data tensor formatting: Successfully converts from 1D/2D arrays to 3D tensor format (n_sequences, max_length, n_features) expected by pomegranate 1.x
• Correct distribution API: Clean migration from pom.NormalDistribution to Normal([mean], [std], covariance_type="diag", frozen=..., inertia=...)
• Modern imports: Switched from wildcard imports to specific imports (from pomegranate.distributions import Normal)

Robust Implementation

• Version checking: Added proper version compatibility check (lines 29-33) with clear error messages
• Error handling: Good error handling for missing pomegranate and invalid observations
• Data validation: Filters empty observations that could cause tensor shape issues
• Type safety: Explicit dtype=np.float32 for PyTorch backend compatibility

Performance Improvements

• Reduced max_iter: Changed from 100000 to 100 (line 15) with comment explaining PyTorch backend performance
• Constants defined: Well-organized configuration constants at the top
• Efficient tensor operations: Proper 3D tensor creation and padding for batch processing

🔧 Technical Improvements Made

1. Data Format Conversion:

   • Old: Simple 1D arrays passed directly
   • New: 3D tensors with proper padding and sequence length tracking (lines 97-103, 219-225)

2. Model Fitting:

   • Old: model.fit(sequences=observations, weights=...)
   • New: model.fit(X=X_3d, sample_weight=sequence_lengths) (line 227)

3. Prediction Handling:

   • Proper extraction of valid sequence parts after prediction (lines 108-111)
   • Handles padded tensor predictions correctly

⚠️ Minor Areas for Future Improvement

1. Configuration Flexibility: Some values like inertia=0.8 are hardcoded in distribution creation - could be made configurable
2. Memory Usage: The 3D tensor padding approach may use more memory for very uneven sequence lengths
3. Documentation: While the code is well-commented, some of the tensor reshaping logic could benefit from additional inline documentation

🧪 Test Coverage Assessment

The failing tests mentioned in comments (test_batch and test_segment_hmm) should now pass with this implementation since:

• The API incompatibility has been resolved
• Proper tensor formatting is implemented
• Model fitting uses correct parameter names

🛡️ Security & Quality

• No security concerns: This is computational genomics code with appropriate data handling
• Good error boundaries: Proper exception handling for missing dependencies
• Type safety: Explicit numpy dtypes prevent potential type-related issues
• Validation: Input validation for empty observations and invalid data

📈 Performance Considerations

• Positive: Reduced training iterations should improve performance
• Positive: Batch processing of sequences is more efficient
• Consideration: 3D tensor padding may increase memory usage for highly variable sequence lengths
• PyTorch compatibility: Explicit float32 dtypes ensure compatibility with PyTorch backend

📋 Dependency Management

The version updates are appropriate:

• pomegranate >=1.0.0 in core.txt
• pomegranate == 1.0.0 in min.txt
• Consistent updates across conda environment files

🏆 Overall Assessment

This is a high-quality migration that successfully addresses the pomegranate 1.x compatibility issues. The code demonstrates:

• Excellent understanding of both old and new APIs
• Thoughtful implementation of tensor formatting requirements
• Good software engineering practices with proper error handling and validation
• Performance awareness with reduced iteration limits and efficient data structures

The implementation should resolve the test failures and provide a solid foundation for using pomegranate 1.x in CNVkit's HMM segmentation functionality.

Recommendation: ✅ Approve - This PR successfully migrates to pomegranate 1.x with robust implementation and should resolve the existing test failures.

---

[claude[bot]]

Claude finished @etal's task —— View job

---

Code Review: Pomegranate 1.x API Migration

Todo List:

• [x] Read and analyze the main HMM implementation changes in cnvlib/segmentation/hmm.py
• [x] Review dependency version updates in requirements files
• [x] Analyze the failing test cases mentioned in comments
• [x] Check for potential bugs and API compatibility issues
• [x] Assess code quality and best practices
• [x] Provide feedback on performance and security considerations
• [x] Evaluate test coverage implications

Analysis Summary

This PR successfully migrates CNVkit from pomegranate <1.0 to pomegranate 1.x API. The migration addresses critical compatibility issues and modernizes the HMM implementation. Here's my comprehensive review:

✅ Excellent Migration Work

API Migration Quality

• Complete API overhaul: The migration from pomegranate <1.0 to 1.x is comprehensive and well-executed
• Proper data tensor formatting: Successfully converts from 1D/2D arrays to 3D tensor format (n_sequences, max_length, n_features) expected by pomegranate 1.x
• Correct distribution API: Clean migration from pom.NormalDistribution to Normal([mean], [std], covariance_type="diag", frozen=..., inertia=...)
• Modern imports: Switched from wildcard imports to specific imports (from pomegranate.distributions import Normal)

Robust Implementation

• Version checking: Added proper version compatibility check (cnvlib/segmentation/hmm.py:29-33) with clear error messages
• Error handling: Good error handling for missing pomegranate and invalid observations
• Data validation: Filters empty observations that could cause tensor shape issues (hmm.py:86-87, 208-211)
• Type safety: Explicit dtype=np.float32 for PyTorch backend compatibility

Performance Improvements

• Reduced max_iter: Changed from 100000 to 100 (hmm.py:15) with comment explaining PyTorch backend performance
• Constants defined: Well-organized configuration constants at the top (hmm.py:14-20)
• Efficient tensor operations: Proper 3D tensor creation and padding for batch processing (hmm.py:96-103, 219-225)

🔧 Technical Improvements Made

1. Data Format Conversion:

   • Old: Simple 1D arrays passed directly
   • New: 3D tensors with proper padding and sequence length tracking (hmm.py:97-103, 219-225)

2. Model Fitting:

   • Old: model.fit(sequences=observations, weights=...)
   • New: model.fit(X=X_3d, sample_weight=sequence_lengths) (hmm.py:227)

3. Prediction Handling:

   • Proper extraction of valid sequence parts after prediction (hmm.py:108-111)
   • Handles padded tensor predictions correctly

⚠️ Minor Areas for Future Improvement

1. Configuration Flexibility: Some values like inertia=0.8 are hardcoded in distribution creation - could be made configurable
2. Memory Usage: The 3D tensor padding approach may use more memory for very uneven sequence lengths
3. Documentation: While the code is well-commented, some of the tensor reshaping logic could benefit from additional inline documentation

🧪 Test Coverage Assessment

The failing tests mentioned in comments (test_batch and test_segment_hmm) should now pass with this implementation since:

• The API incompatibility has been resolved
• Proper tensor formatting is implemented
• Model fitting uses correct parameter names

🛡️ Security & Quality

• No security concerns: This is computational genomics code with appropriate data handling
• Good error boundaries: Proper exception handling for missing dependencies
• Type safety: Explicit numpy dtypes prevent potential type-related issues
• Validation: Input validation for empty observations and invalid data

📈 Performance Considerations

• Positive: Reduced training iterations should improve performance
• Positive: Batch processing of sequences is more efficient
• Consideration: 3D tensor padding may increase memory usage for highly variable sequence lengths
• PyTorch compatibility: Explicit float32 dtypes ensure compatibility with PyTorch backend

📋 Dependency Management

The version updates are appropriate:

• pomegranate >=1.0.0 in requirements/hmm.txt
• pomegranate == 1.0.0 in requirements/min.txt
• Consistent updates across conda environment files
• Well-structured separation of HMM dependencies into optional extras (requirements/hmm.txt)

📚 Documentation Excellence

• New HMM_README.md: Comprehensive documentation explaining installation, usage, and performance considerations
• Clear migration path: Good explanation of why dependencies are optional and performance trade-offs
• Conda environment: Proper environment file for HMM functionality

🏆 Overall Assessment

This is a high-quality migration that successfully addresses the pomegranate 1.x compatibility issues. The code demonstrates:

• Excellent understanding of both old and new APIs
• Thoughtful implementation of tensor formatting requirements
• Good software engineering practices with proper error handling and validation
• Performance awareness with reduced iteration limits and efficient data structures
• User-friendly approach with clear documentation and optional installation

The implementation should resolve the test failures and provide a solid foundation for using pomegranate 1.x in CNVkit's HMM segmentation functionality.

Recommendation: ✅ Approve - This PR successfully migrates to pomegranate 1.x with robust implementation and should resolve the existing test failures.

---

[claude[bot]]

Claude finished @etal's task —— View job

---

Code Review: Pomegranate 1.x API Migration

Todo List:

• [x] Read and analyze the main HMM implementation changes in cnvlib/segmentation/hmm.py
• [x] Review dependency version updates in requirements files
• [x] Analyze the failing test cases mentioned in comments
• [x] Check for potential bugs and API compatibility issues
• [x] Assess code quality and best practices
• [x] Provide feedback on performance and security considerations
• [x] Evaluate test coverage implications

Analysis Summary

This PR successfully migrates CNVkit from pomegranate <1.0 to pomegranate 1.x API. The migration addresses critical compatibility issues and modernizes the HMM implementation. Here's my comprehensive review:

✅ Excellent Migration Work

API Migration Quality

• Complete API overhaul: The migration from pomegranate <1.0 to 1.x is comprehensive and well-executed
• Proper data tensor formatting: Successfully converts from 1D/2D arrays to 3D tensor format (n_sequences, max_length, n_features) expected by pomegranate 1.x
• Correct distribution API: Clean migration from pom.NormalDistribution to Normal([mean], [std], covariance_type="diag", frozen=..., inertia=...)
• Modern imports: Switched from wildcard imports to specific imports (from pomegranate.distributions import Normal)

Robust Implementation

• Version checking: Added proper version compatibility check (cnvlib/segmentation/hmm.py:29-33) with clear error messages
• Error handling: Good error handling for missing pomegranate and invalid observations
• Data validation: Filters empty observations that could cause tensor shape issues (hmm.py:86-87, 208-211)
• Type safety: Explicit dtype=np.float32 for PyTorch backend compatibility

Performance Improvements

• Reduced max_iter: Changed from 100000 to 100 (hmm.py:15) with comment explaining PyTorch backend performance
• Constants defined: Well-organized configuration constants at the top (hmm.py:14-20)
• Efficient tensor operations: Proper 3D tensor creation and padding for batch processing (hmm.py:96-103, 219-225)

🔧 Technical Improvements Made

1. Data Format Conversion:

   • Old: Simple 1D arrays passed directly
   • New: 3D tensors with proper padding and sequence length tracking (hmm.py:97-103, 219-225)

2. Model Fitting:

   • Old: model.fit(sequences=observations, weights=...)
   • New: model.fit(X=X_3d, sample_weight=sequence_lengths) (hmm.py:227)

3. Prediction Handling:

   • Proper extraction of valid sequence parts after prediction (hmm.py:108-111)
   • Handles padded tensor predictions correctly

⚠️ Minor Areas for Future Improvement

1. Configuration Flexibility: Some values like inertia=0.8 are hardcoded in distribution creation - could be made configurable
2. Memory Usage: The 3D tensor padding approach may use more memory for very uneven sequence lengths
3. Documentation: While the code is well-commented, some of the tensor reshaping logic could benefit from additional inline documentation

🧪 Test Coverage Assessment

The failing tests mentioned in comments (test_batch and test_segment_hmm) should now pass with this implementation since:

• The API incompatibility has been resolved
• Proper tensor formatting is implemented
• Model fitting uses correct parameter names

🛡️ Security & Quality

• No security concerns: This is computational genomics code with appropriate data handling
• Good error boundaries: Proper exception handling for missing dependencies
• Type safety: Explicit numpy dtypes prevent potential type-related issues
• Validation: Input validation for empty observations and invalid data

📈 Performance Considerations

• Positive: Reduced training iterations should improve performance
• Positive: Batch processing of sequences is more efficient
• Consideration: 3D tensor padding may increase memory usage for highly variable sequence lengths
• PyTorch compatibility: Explicit float32 dtypes ensure compatibility with PyTorch backend

📋 Dependency Management

The version updates are appropriate:

• pomegranate >=1.0.0 in requirements/hmm.txt
• pomegranate == 1.0.0 in requirements/min.txt
• Consistent updates across conda environment files
• Well-structured separation of HMM dependencies into optional extras (requirements/hmm.txt)

📚 Documentation Excellence

• New HMM_README.md: Comprehensive documentation explaining installation, usage, and performance considerations
• Clear migration path: Good explanation of why dependencies are optional and performance trade-offs
• Conda environment: Proper environment file for HMM functionality

🏆 Overall Assessment

This is a high-quality migration that successfully addresses the pomegranate 1.x compatibility issues. The code demonstrates:

• Excellent understanding of both old and new APIs
• Thoughtful implementation of tensor formatting requirements
• Good software engineering practices with proper error handling and validation
• Performance awareness with reduced iteration limits and efficient data structures
• User-friendly approach with clear documentation and optional installation

The implementation should resolve the test failures and provide a solid foundation for using pomegranate 1.x in CNVkit's HMM segmentation functionality.

Recommendation: ✅ Approve - This PR successfully migrates to pomegranate 1.x with robust implementation and should resolve the existing test failures.

---

[claude[bot]]

Claude finished @etal's task —— View job

---

Code Review: Pomegranate 1.x API Migration

Todo List:

• [x] Read and analyze the main HMM implementation changes in cnvlib/segmentation/hmm.py
• [x] Review dependency version updates in requirements files
• [x] Analyze the failing test cases mentioned in comments
• [x] Check for potential bugs and API compatibility issues
• [x] Assess code quality and best practices
• [x] Provide feedback on performance and security considerations
• [x] Evaluate test coverage implications

Analysis Summary

This PR successfully migrates CNVkit from pomegranate <1.0 to pomegranate 1.x API. The migration addresses critical compatibility issues and modernizes the HMM implementation. Here's my comprehensive review:

✅ Excellent Migration Work

API Migration Quality

• Complete API overhaul: The migration from pomegranate <1.0 to 1.x is comprehensive and well-executed
• Proper data tensor formatting: Successfully converts from 1D/2D arrays to 3D tensor format (n_sequences, max_length, n_features) expected by pomegranate 1.x
• Correct distribution API: Clean migration from pom.NormalDistribution to Normal([mean], [std], covariance_type="diag", frozen=..., inertia=...)
• Modern imports: Switched from wildcard imports to specific imports (from pomegranate.distributions import Normal)

Robust Implementation

• Version checking: Added proper version compatibility check (cnvlib/segmentation/hmm.py:29-33) with clear error messages
• Error handling: Good error handling for missing pomegranate and invalid observations
• Data validation: Filters empty observations that could cause tensor shape issues (hmm.py:86-87, 208-211)
• Type safety: Explicit dtype=np.float32 for PyTorch backend compatibility

Performance Improvements

• Reduced max_iter: Changed from 100000 to 100 (hmm.py:15) with comment explaining PyTorch backend performance
• Constants defined: Well-organized configuration constants at the top (hmm.py:14-20)
• Efficient tensor operations: Proper 3D tensor creation and padding for batch processing (hmm.py:96-103, 219-225)

🔧 Technical Improvements Made

1. Data Format Conversion:

   • Old: Simple 1D arrays passed directly
   • New: 3D tensors with proper padding and sequence length tracking (hmm.py:97-103, 219-225)

2. Model Fitting:

   • Old: model.fit(sequences=observations, weights=...)
   • New: model.fit(X=X_3d, sample_weight=sequence_lengths) (hmm.py:227)

3. Prediction Handling:

   • Proper extraction of valid sequence parts after prediction (hmm.py:108-111)
   • Handles padded tensor predictions correctly

⚠️ Minor Areas for Future Improvement

1. Configuration Flexibility: Some values like inertia=0.8 are hardcoded in distribution creation - could be made configurable
2. Memory Usage: The 3D tensor padding approach may use more memory for very uneven sequence lengths
3. Documentation: While the code is well-commented, some of the tensor reshaping logic could benefit from additional inline documentation

🧪 Test Coverage Assessment

The failing tests mentioned in comments (test_batch and test_segment_hmm) should now pass with this implementation since:

• The API incompatibility has been resolved
• Proper tensor formatting is implemented
• Model fitting uses correct parameter names

🛡️ Security & Quality

• No security concerns: This is computational genomics code with appropriate data handling
• Good error boundaries: Proper exception handling for missing dependencies
• Type safety: Explicit numpy dtypes prevent potential type-related issues
• Validation: Input validation for empty observations and invalid data

📈 Performance Considerations

• Positive: Reduced training iterations should improve performance
• Positive: Batch processing of sequences is more efficient
• Consideration: 3D tensor padding may increase memory usage for highly variable sequence lengths
• PyTorch compatibility: Explicit float32 dtypes ensure compatibility with PyTorch backend

📋 Dependency Management

The version updates are appropriate:

• pomegranate >=1.0.0 in requirements/hmm.txt
• pomegranate == 1.0.0 in requirements/min.txt
• Consistent updates across conda environment files
• Well-structured separation of HMM dependencies into optional extras (requirements/hmm.txt)

📚 Documentation Excellence

• New HMM_README.md: Comprehensive documentation explaining installation, usage, and performance considerations
• Clear migration path: Good explanation of why dependencies are optional and performance trade-offs
• Conda environment: Proper environment file for HMM functionality

🏆 Overall Assessment

This is a high-quality migration that successfully addresses the pomegranate 1.x compatibility issues. The code demonstrates:

• Excellent understanding of both old and new APIs
• Thoughtful implementation of tensor formatting requirements
• Good software engineering practices with proper error handling and validation
• Performance awareness with reduced iteration limits and efficient data structures
• User-friendly approach with clear documentation and optional installation

The implementation should resolve the test failures and provide a solid foundation for using pomegranate 1.x in CNVkit's HMM segmentation functionality.

Recommendation: ✅ Approve - This PR successfully migrates to pomegranate 1.x with robust implementation and should resolve the existing test failures.

---

[etal]

The deed is done. I apologize for the bot spam.

@mr-c -- I've split CNVkit into two pip/conda packages, where the only difference is whether the pomegranate and pytorch dependencies are required; the Python codebase itself is the same either way. For Debian, assuming pytorch and pomegranate 1.0+ are already available as a packages, I think I'd recommend including the HMM dependencies and ignoring the split.

Without the pomegranate dependency (default behavior with pip install cnvkit going forward), if you run segmentation with the HMM method (-m hmm), it will fail and report the missing dependency. With the dependency (pip install cnvkit[hmm]), it should all work as you'd expect.