Merged
Conversation
Introduce apply_cutoffs and geometric_mean_peak to merge multiple peaks per interval (uses a weighted geometric mean for position and sums heights). Update _prepare_non_separate_nii to warn when cutoffs are not applied and to auto-apply provided cutoffs. Add did_apply_cutoffs flag and store cutoffs; adjust imports accordingly.
Use params.fit_model.fit_t1 to decide T1 output in IVIM In NNLS: swap D/f assignment to match _get_peak_stats return, add explicit array types, fix indices extraction from np.where, and normalize fractional amplitudes so they sum to 1. Add TODO for curve and spectrum calculations.
Add unit tests for _prepare_non_separate_nii and _prepare_separate_nii, plus new test classes covering separate_files behavior and dtype handling. Replace direct on-disk file assertions with checks on returned paths and image objects.
Add extensive test coverage for NNLS results including: - Cutoff application and fraction preservation - NIfTI preparation methods with cutoff handling - Complete save workflows (NIfTI, Excel, spectrum) - Geometric mean peak calculations - Integration tests and edge cases Validates centralized NIfTI saving with NNLS cutoff requirements.
Remove duplicate helpers from tests/test_results.py and add get_spectrum plus fixtures results_biexp_pixel, biexp_results_segmentation, and results_with_t1 to tests/conftest.py. Adjust imports accordingly.
Initialize start_values, constraints, and constraint_types properly for the individual boundary mode (use parallel arrays converted to float32/int32).
Move numpy import and clean up whitespace. Use numeric keys in IVIMBoundaryDict for individual boundaries and update tight-bound keys and assertions to match the new parameter ordering. Add the mocker fixture and patch the ivim params model to test invalid model handling.
darksim33
commented
Jan 19, 2026
Owner
Author
darksim33
left a comment
There was a problem hiding this comment.
Summary
This PR refactors the NIfTI saving mechanism to use a centralized approach and adds comprehensive test coverage for NNLS results, particularly focusing on cutoff handling and the new preparation methods.
Changes
1. Centralized NIfTI Saving (results.py, ivim.py, nnls.py)
Refactored saving architecture:
- Introduced preparation methods that separate file path/image preparation from actual saving
_prepare_non_separate_nii(): Prepares combined 4D files (e.g.,_d.nii.gzwith all D values)_prepare_separate_nii(): Prepares indexed 3D files (e.g.,_d_0.nii.gz,_d_1.nii.gz)- Both methods return
tuple[list[Path], list[RadImgArray]] save_to_nii()now calls preparation methods then uses centralizednifti.arrays_to_nifti()
Benefits:
- Single point of control for NIfTI file writing
- Consistent file naming across all result types
- Easier to extend functionality in subclasses
- Better separation of concerns (prepare vs. save)
IVIM-specific enhancements:
- Override preparation methods to add T1 parameter when fitted
- T1 saved as
{basename}_t1.nii.gz - Works in both separate and non-separate modes
NNLS-specific enhancements:
- Override
_prepare_non_separate_nii()to handle cutoff requirements - Automatically apply cutoffs if passed via
kwargs - Log warning if cutoffs not applied (NNLS needs normalized compartment counts)
- Prevents inconsistent NIfTI files with varying dimensions
2. Enhanced NNLS Test Coverage (test_nnls_results.py)
New test classes and coverage:
TestNNLSCutoffs (7 tests)
- Basic cutoff application and validation
- Fraction preservation with controlled spectra
- Edge cases: no peaks in range, multiple peaks in range
- Overlapping cutoff ranges
- Cutoff attribute storage
TestNNLSNIfTIPreparation (4 tests)
- Preparation methods with applied cutoffs
- Warning when cutoffs not applied (loguru logger compatible)
- Cutoff application via kwargs
- Prevention of cutoff reapplication
TestNNLSNIfTISaving (3 tests)
- Full
save_to_nii()workflow with cutoffs - Separate files mode with cutoffs
- dtype preservation validation
TestNNLSSpectrum (4 tests)
- Spectrum storage after
eval_results() - Excel export with correct bins
- Custom bins functionality
- Spectrum shape validation
TestGeometricMeanPeak (3 tests)
- Basic geometric mean calculation
- Equal weights scenario
- Single peak edge case
TestNNLSIntegration (2 tests)
- Complete workflow: evaluate → cutoff → save
- Multi-format saving (NIfTI + Excel + spectrum)
TestNNLSEdgeCases (5 tests)
- Empty results handling
- Single peak detection
- Very narrow cutoff ranges
- Cutoffs extending beyond bin range
Test improvements:
- Uses controlled spectra with known peak positions
- Validates fraction preservation across transformations
- Ensures proper handling of NaN values in sparse ranges
- Tests both pixel-wise and segmentation-wise workflows
3. Documentation Updates (DataExport.md)
Comprehensive export documentation:
- Detailed NIfTI section with file naming conventions
- Examples for both combined and separate file modes
- NNLS cutoff usage examples and explanations
- IVIM T1 parameter documentation
- Excel export formats and use cases
- Spectrum and fitted curve export examples
- Format comparison table
- Best practices and troubleshooting guide
4. Bug Fixes
GPU fitting test fix (test_pygpufit.py):
- Fixed
test_gpu_fitter_raises_on_invalid_modelto properly mock themodelproperty - Used
unittest.mock.patchwithPropertyMockto mock read-only property
Breaking Changes
None. The API remains unchanged and is fully backwards compatible.
Testing
- ✅ All existing tests pass
- ✅ 28 new NNLS tests added
- ✅ Test coverage for preparation methods
- ✅ Edge case coverage significantly improved
- ✅ Integration tests validate complete workflows
Migration Guide
No migration needed - existing code continues to work as before.
Optional enhancement for NNLS users:
# New: Pass cutoffs directly during save
results.save_to_nii('output', img, dtype=float, cutoffs=[(0.0, 0.001), (0.001, 0.01)])
# Still works: Apply cutoffs separately
results.apply_cutoffs([(0.0, 0.001), (0.001, 0.01)])
results.save_to_nii('output', img, dtype=float)Checklist
- Code follows project style guidelines
- Tests added for new functionality
- Documentation updated
- All tests passing
- Backwards compatible
- Commit messages follow conventional commits format
Closed
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
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.
Introduce helper functions for saving NIfTI files and improve NNLS results processing with cutoff handling and peak merging. Refactor tests for better coverage and documentation updates. Fix various issues related to GPU fitting and peak handling.