Honour VisuGroupDepVals valsStart when two frame groups share an array - #208
Merged
Merged
Conversation
`ruff format --check` reported twelve files as unformatted, so any change touching one of them dragged unrelated reflow into its diff. Run the formatter once, on its own, so subsequent changes show only what they actually change. Formatting only: no behaviour changes, and `ruff check` is clean before and after. The suite is unchanged at 2140 passed, 12 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SNrm3jFhpTPShP8xGkePC
frame_group_values read only the name element of each VisuGroupDepVals entry and never element [1], which spec 7.4 defines as VISU_DEPVALS_TYPE.valsStart -- the offset at which that frame group's block begins inside the dependent parameter's array. When two groups declare a dependency on the same parameter the writer concatenates their blocks and gives each group its own valsStart. Recording both axes under the one name made the code spread the concatenated array over the product of both axis lengths; the product never matched, and the parameter was dropped from the result entirely. On the Cyceron DWI PROCNOs that silently lost VisuFGElemComment -- the per-map names (Fractional Anisotropy, Trace, the tensor components, the eigenvalue and eigenvector maps) that are the only thing saying which of the 22 volumes is which. Spec 7.4 points at VisuFGElemComment as exactly where those labels live. Give each (group, dependency) pair its own window. A parameter owned by one group keeps its plain name, so nothing that worked before changes; a shared one is keyed per group, which is the only way a single mapping can express two windows. Across 514 corpus reconstructions the result changes for exactly three, all of them gaining the labels they used to lose. Closes isi-nmr#200 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017SNrm3jFhpTPShP8xGkePC
gdevenyi
force-pushed
the
fix/frame-group-vals-start
branch
from
August 8, 2026 20:51
b3198df to
a56454e
Compare
Contributor
Author
|
Rebased onto #213 ( Integration check across all eleven conformance PRs merged together: 2164 passed, 12 skipped; corpus load test 3202 → 3207 of 3478 with zero newly broken datasets. |
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.
Fixes #200.
Spec 7.4:
frame_group_valuesread onlynameand nevervalsStart. When two groups depend on the same parameter the writer concatenates their blocks and gives each group its own start; recording both axes under one name made the code spread the concatenation over the product of the two axis lengths, which never matched, so the parameter was dropped.Before / after
resources/testdata/bruker2nifti_qa/raw/Cyceron_DWI/.../3/pdata/2—FG_DTI(len 22,valsStart3),FG_CYCLE(len 3,valsStart0), 25 comments:Change
Each
(group, dependency)pair gets its own windowparameter[valsStart : valsStart + group_len]. A parameter owned by one group keeps its plain name, so nothing that worked before changes; a shared one is keyedname[GROUP_ID], which is the only way a single mapping can express two windows. The alignment logic moved into_align_to_frame_groupsunchanged.Across 514 corpus reconstructions the result changes for exactly three —
Cyceron_DWI/3/pdata/1,3/pdata/2andpv5/0.2H2/31/pdata/2— and every change is an addition; nothing was removed or altered.Tests
test_two_frame_groups_can_share_one_dependent_parameter— a synthetic two-group reconstruction whoseVisuFGElemCommentis shared with different starts. Fails on master withKeyError.test_a_parameter_owned_by_one_frame_group_keeps_its_plain_name— guards the unchanged single-group key.Suite: 2142 passed, 12 skipped.