Repository navigation
Fix MRDS output corruption and orientation handling - #1358
Merged
Merged
Conversation
…tatefulImage, requires the header/dtype fix so uint8 NUFO references do not silently quantize float outputs
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Preserve source metadata and filename when reorienting non-RAS images, and add regression coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Updates MRDS tools to use StatefulImage for orientation-safe processing and correct output precision.
Changes:
- Reorient MRDS inputs and masks consistently.
- Preserve output orientation and data types.
- Add orientation, dtype, and metadata regression tests.
| File | Summary |
|---|---|
src/scilpy/io/tests/test_stateful_image.py |
Tests dtype, header, metadata, and orientation handling. |
src/scilpy/io/stateful_image.py |
Extends orientation, metadata, and dtype handling. |
src/scilpy/cli/tests/test_mrds_select_number_of_tensors.py |
Tests tensor selection across orientations. |
src/scilpy/cli/tests/test_mrds_metrics.py |
Tests MRDS metrics with LAS/RAS inputs. |
src/scilpy/cli/scil_mrds_select_number_of_tensors.py |
Adds orientation-safe MRDS selection and output preservation. |
src/scilpy/cli/scil_mrds_metrics.py |
Adds orientation-safe metric generation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ile_map regression test
…it_branch_4_mrds
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev_3.0.x #1358 +/- ##
=============================================
+ Coverage 72.95% 73.03% +0.07%
=============================================
Files 304 304
Lines 27053 27097 +44
Branches 3839 3845 +6
=============================================
+ Hits 19737 19789 +52
+ Misses 5707 5698 -9
- Partials 1609 1610 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
frheault
changed the title
CHrlS98
requested changes
Oct 21, 2026
Comment on lines
+110
to
+114
| # and test runs. On this branch, PIR mask is reoriented correctly | ||
| # to match the LAS dataset, so the rendered peak glyphs on the mid-slice | ||
| # match the reference exactly. | ||
| # On dev_3.0.x, scil_mrds_select_number_of_tensors crashes with IndexError. | ||
| ret_viz_ref = script_runner.run([ |
Contributor
There was a problem hiding this comment.
this is a comment that should not be included in the source code (the "On this branch..." part).
arnaudbore
approved these changes
Oct 21, 2026
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.
Quick description
Depends on PR 1 (
split_branch_1_core), included here so this PR's tests pass on their own (see why below).Updates
scil_mrds_metricsandscil_mrds_select_number_of_tensorsto scilpy's newer image-loading code, making both robust to any input orientation.While testing this in isolation, a real bug turned up in
scil_mrds_select_number_of_tensors: it could silently write its output (eigenvector directions, signal fractions, etc.) using the wrong numeric precision, because it borrowed file-type information from the NUFO/label map instead of its own data. This quietly rounded/corrupted output values by 1-3%, with no crash or warning. Fixed by PR 1's dtype fix, included here.I validated the before/after behavior using this data/code.
Type of change
Provide data, screenshots, command line to test (if relevant)
pytest src/scilpy/cli/tests/test_mrds_metrics.py src/scilpy/cli/tests/test_mrds_select_number_of_tensors.py src/scilpy/io/tests/test_stateful_image.py -v46 passed.
Checklist
🤖 Generated with Claude Code