BUG: Fix image_from_array shape handling for F-contiguous arrays#5775
BUG: Fix image_from_array shape handling for F-contiguous arrays#5775
Conversation
eb57af4 to
71ba10c
Compare
71ba10c to
5167f98
Compare
|
@copilot address failing ci tests |
7c49c8a to
3af7110
Compare
3af7110 to
5980cc0
Compare
|
Took over this branch — rebased onto current Why deep-copy instead of conditional reversalThe C++ side at The conditional-reversal approach ( Deep-copying the input via
The cost is a copy for F-arrays. The existing fallback for fully non-contiguous arrays already does this with a warning; the same warning now covers F-arrays. Test changes
|
This comment was marked as resolved.
This comment was marked as resolved.
5980cc0 to
2f75932
Compare
|
@thewtex. --- working through old PR's, trying to address outstanding issues. You triggered the initial co-pilot start to this PR. I pushed to a green state. I'm not sure if this is the approach you originally had anticipated, but it looks like a reasonable state to me. |
cd93817 to
2181a21
Compare
|
Rebased onto current |
2181a21 to
ea5b650
Compare
|
Force-pushed What was missing on the prior PR HEADTwo greptile P2 threads were marked "Addressed in
Additional items the careful review surfaced
Final commitsTwo clean logical commits on top of fresh |
Deep-copy any non-C-contiguous input via np.ascontiguousarray() before constructing the ITK image view, so image_from_array(arr) and image_from_array(arr.T) produce ITK images with identical NumPy-reversed shape semantics regardless of the input's memory layout. Update test_NumPyBridge_FortranOrder to assert the now- consistent ITK[i,j] == NumPy[j,i] mapping for F-contiguous inputs; the previous assertions encoded the prior double-reversal behavior where C++ and Python both reversed size for F buffers.
image_from_array(arr.T) used to produce the same image as image_from_array(arr) (transpose was a no-op). After the preceding BUG fix, the .T input is deep-copied to C order so the resulting image has reversed dimensions vs. the original, and the rule is uniform: itk.size(image) == array.shape[::-1] regardless of the input's memory layout. Add a migration-guide entry covering the rationale, the new shape rule, and the audit list users should walk through (callers that drop the .T to recover the prior behavior, callers that round-trip through ITK, and callers that should silence the new non-C-contiguous warning via need_contiguous=False).
ea5b650 to
fdee9cb
Compare
|
Force-pushed What changedCode (
Tests (
Migration guide (
Final shapeBoth commits squashed clean (no |
The pinned v0.8 (2018-2019) ships an action that builds the Go binary from source on every CI run. Each build downloads dependencies from gopkg.in, which intermittently fails the TLS handshake and aborts the labeling job. The failure has been observed across multiple recent PRs (InsightSoftwareConsortium#5775, InsightSoftwareConsortium#6177, InsightSoftwareConsortium#6164 -- 'Docker build failed with exit code 1' from 'go: gopkg.in/check.v1: net/http: TLS handshake timeout'). srvaroa/labeler ships a pre-built Docker image starting at v1.8.1, so newer versions skip the Go-compile step entirely. The maintainer publishes a floating 'v1' major tag that auto-tracks minor/patch releases; v1.14.0 is current upstream. The v1 schema replaces the v0.8-era flat dict-of-rules with a 'version: 1' header + 'labels:' list of {label, title, files, ...} objects, so .github/labeler.yml is migrated alongside the workflow bump. Pattern semantics are unchanged: both v0.8 and v1 use Go's RE2 regex engine, matching unanchored against PR file paths -- so existing patterns like 'Modules/IO/*' (which is regex 'Modules/IO/' followed by zero-or-more '/') continue to behave the same in v1.
hjmjohnson
left a comment
There was a problem hiding this comment.
I think this approach is correct. The migration guide indicates the backward compatibility, but the interface now looks like what I would expect.
It is almost like a self-review. Should let another person approve.
Description
itk.image_from_array()incorrectly preserved the original array shape when given F-contiguous inputs (e.g.,array.T), whileitk.image_view_from_array()correctly used the transposed shape. This inconsistency broke user expectations and made the two functions behave differently for the same input.Root cause: Double reversal in shape handling
ndarr.shape[::-1]Fix: Conditional shape reversal based on contiguity
This matches the existing vector image logic and aligns both functions.
Changes
Modules/Bridge/NumPy/wrapping/PyBuffer.i.in: Added contiguity check before shape reversal inGetImageViewFromArray()Wrapping/Generators/Python/Tests/extras.py: Added tests for transposed array handling with black-compliant formattingExample
PR Checklist
Original prompt
itk.image_from_arraywhen passingarray.T#4329💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.