Skip to content

BUG: Compare NIfTI test output by content, not by compressed bytes - #31

Merged
hjmjohnson merged 1 commit into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/test-gz-content-compare
Sep 21, 2026
Merged

hjmjohnson merged 1 commit into
InsightSoftwareConsortium:masterfrom
gdevenyi:pr/test-gz-content-compare

Conversation

@gdevenyi

@gdevenyi gdevenyi commented Aug 15, 2026 •

Copy link
Copy Markdown

nifti_c22_copy_image has been failing on this machine since before any of
this work started. It converts an image i16 -> i64 -> i16 and asserts
the result matches the original, using

cmp out.c22.0.i16.nii.gz out.c22.2.0.i16.nii.gz

That compares gzip output, which is not reproducible across zlib
implementations. The system zlib here is zlib-ng 1.3.1, which Arch,
CachyOS and a growing number of distributions ship in place of stock
zlib; it encodes the same input differently. Both files come out at
exactly 642454 bytes and differ from byte 321594 on.

The conversion itself is fine. Decompressed, the two files are
byte-identical at 1114768 bytes each, so the round-trip through int64 and
back preserves the data exactly, which is what the test set out to check.
Confirmed the other way too: in an Ubuntu 24.04 container with stock
zlib 1.3, the test passes unmodified.

The test now decompresses before comparing, via a small nii_cmp helper
that falls back to plain cmp for uncompressed files. With this the suite
is 92 of 92 on both zlib-ng and stock zlib; previously it was 91 of 92
on any zlib-ng system.


Interface impact: none. On the union of all these changes, configured with USE_FSL_CODE=ON and USE_CIFTI_CODE=ON: all 448 exported symbols across libniftiio, libnifti2, libznz, libfslio, libnifticdf and libcifti are identical to master under nm -D --defined-only, and all ten installed headers are identical under gcc -E -P. Under gcc -dM -E one macro definition differs, intentionally and only in text: #61 makes FSL_RADIOLOGICAL read (-1) so it is safe inside an expression. Its value is still -1, checked by compiling against each installed fslio.h and printing it.

Verification. This branch: builds with gcc 16.1.1, ctest unchanged from master (2 of 345 fail on master itself in this environment; #31 and #29 each fix one). The union of all the PRs: 0 errors under both gcc 16.1.1 and clang 22.1.8, ctest 345/345 under each, and the whole suite under valgrind memcheck with --trace-children=yes gives 484 traced processes with no invalid access, no uninitialised value and no leak in any nifti binary.

Coordination. Every line of every branch was compared, whitespace-normalised, against the diffs of the open PRs (#11, #21, #22, #23, #24). Where one of those already changes a line, the line was left alone, and the few deliberate overlaps are named in the text above. What survives is 17 compiler warnings, all of them on those lines: 9 -Wsign-conversion (5 in fslio.c for #22, 2 in nifti2_io.c and 2 in nifti_tester001.c for #24) and 8 -Wcalloc-transposed-args in nifti_findhdrname and nifti_findimgname, which #11 rewrites. No formatting changes appear anywhere, to stay clear of #10 and #12.

One of a set of independent, single-purpose PRs. Each bases on master and can be merged on its own, in any order.

The full set of PRs (35)

The union of all of them is on the fork as all-changes, if you want to build and test the lot at once.

CI and build

Configuration and documentation

Defects

Warning and check classes

nifti_c22_copy_image has been failing on this machine since before any of
this work started.  It converts an image i16 -> i64 -> i16 and asserts
the result matches the original, using

    cmp out.c22.0.i16.nii.gz out.c22.2.0.i16.nii.gz

That compares gzip output, which is not reproducible across zlib
implementations.  The system zlib here is zlib-ng 1.3.1, which Arch,
CachyOS and a growing number of distributions ship in place of stock
zlib; it encodes the same input differently.  Both files come out at
exactly 642454 bytes and differ from byte 321594 on.

The conversion itself is fine.  Decompressed, the two files are
byte-identical at 1114768 bytes each, so the round-trip through int64 and
back preserves the data exactly, which is what the test set out to check.
Confirmed the other way too: in an Ubuntu 24.04 container with stock
zlib 1.3, the test passes unmodified.

The test now decompresses before comparing, via a small nii_cmp helper
that falls back to plain cmp for uncompressed files.  With this the suite
is 92 of 92 on both zlib-ng and stock zlib; previously it was 91 of 92
on any zlib-ng system.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KSPnbwpDjVcAYqDdVqLkMU
This was referenced Aug 15, 2026
@hjmjohnson

Copy link
Copy Markdown
Member

Reviewed as part of a sweep over the open backlog. Merge as-is. The diagnosis is correct and the fix is the right shape.

gzip output is not reproducible across zlib implementations, and zlib-ng — which Arch, CachyOS, and other current distributions ship as the system zlib — encodes identical input to different bytes. Comparing .gz files with cmp therefore fails on those systems even though the image data round-trips perfectly: a false failure that says nothing about correctness.

Beyond fixing c22_copy_image, this matters because byte-comparing compressed output makes any future baseline test environment-dependent, and baseline comparison is the foundation for the regression coverage this project is missing.

Follow-up: hoist nii_cmp out of this one script

nii_cmp() is defined locally in c22_copy_image.sh, but the problem it solves is not local to that test — any script that compares compressed NIfTI output has it. A shared location sourced by the scripts in cmake_testscripts/ would stop the next comparison test from reintroducing bare cmp on a .gz.

Not a blocker for this PR, and arguably cleaner as a separate change once the immediate failure is fixed.

Minor: the .raw temporaries are left behind

nii_cmp writes "$1.raw" and "$2.raw" beside the inputs and never removes them. Harmless — it runs in the test output directory — but a rm -f after the compare, or trap-based cleanup, would keep the directory honest for anyone diffing it by hand afterwards.

Also worth a thought: on the decompression-failure path (|| return 1) the function returns the same 1 that a genuine content mismatch returns, so a broken gzip and a real difference are indistinguishable to the caller. Given both are failures that stop the test, this is fine as written; mentioning it only because the distinction would matter if nii_cmp becomes shared.

@hjmjohnson
hjmjohnson merged commit 510abc1 into InsightSoftwareConsortium:master Sep 21, 2026
4 checks passed
@hjmjohnson

Copy link
Copy Markdown
Member

The commit messages in this range were rewritten to remove trailers that do not belong in permanent history: Co-Authored-By: naming an AI tool, and Claude-Session: URLs that resolve for nobody. Only messages changed — the tree at the tip of master is byte-identical, and author, committer, and dates are preserved.

This PR's commit on the rewritten master:

  • 7d92f16249 BUG: Compare NIfTI test output by content, not by compressed bytes

The SHA recorded above by GitHub is from the pre-rewrite history and no longer resolves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants