fix: permute Data._decomposition and _modulo on transpose (fixes #2187)#2938
Open
gaoflow wants to merge 2 commits into
Open
fix: permute Data._decomposition and _modulo on transpose (fixes #2187)#2938gaoflow wants to merge 2 commits into
gaoflow wants to merge 2 commits into
Conversation
A ``Data`` view created by ``.T`` (or ``transpose``/``swapaxes``)
inherited the source's ``_decomposition`` and ``_modulo`` tuples
unchanged via ``__array_finalize__``. Both tuples are indexed by
*axis*, so after the axes are permuted they point at the wrong
dimensions. A subsequent ``__getitem__`` then routes the slice
through ``_index_glb_to_loc`` against the wrong per-axis ranges
and silently returns a wrong-shaped result, e.g.:
grid = Grid(shape=(4, 6))
f = Function(name='f', grid=grid)
f.data.T[::2, ::2].shape # (2, 2) -- wrong, NumPy gives (3, 2)
Overriding ``.T`` alone is not enough because ``ndarray.T`` is a
C-level shortcut that does not dispatch through Python's
``transpose``; ``transpose`` alone is not enough either because
``.T`` then keeps the old (buggy) behavior. So three thin
overrides are added on ``Data``:
* ``transpose(*axes)``: normalize ``axes`` (no-arg, sequence, or
per-arg), call ``super().transpose``, then permute
``_decomposition``/``_modulo`` into the new axis order and
refresh ``_is_distributed`` so it stays consistent.
* ``swapaxes(axis1, axis2)``: same treatment for the two-axis case.
* ``T``: explicit Python property that forwards to ``transpose``.
Slice-then-transpose (``f.data[::2, ::2].T``) already worked
because the slice goes through ``__getitem__`` first and the
transpose only flips a 2-element metadata tuple where both entries
match. The fix preserves that path unchanged.
Adds ``test_slice_after_transpose`` (covers ``.T``, ``transpose``,
``swapaxes``, 2D and 3D, with the NumPy reference array as the
source of truth) and ``test_transpose_permutes_data_metadata``
(asserts ``_decomposition``/``_modulo`` end up in the new order).
Fixes devitocodes#2187.
mloubout
reviewed
May 28, 2026
Contributor
mloubout
left a comment
There was a problem hiding this comment.
Thanks for that, looks sane and definitely a bug. Some small comments
| """ | ||
| if not axes or axes == (None,): | ||
| new_order = tuple(range(self.ndim - 1, -1, -1)) | ||
| elif len(axes) == 1 and isinstance(axes[0], Iterable): |
Contributor
There was a problem hiding this comment.
Can use devito.tools.as_tuple for these two cases
| new_order = tuple(ax % self.ndim for ax in new_order) | ||
|
|
||
| ret = super().transpose(*axes) | ||
| if isinstance(ret, Data): |
| axis1 = axis1 % self.ndim | ||
| axis2 = axis2 % self.ndim | ||
| ret = super().swapaxes(axis1, axis2) | ||
| if isinstance(ret, Data): |
| f = Function(name='f', grid=grid) | ||
| f.data[:] = np.arange(24).reshape((4, 6)).astype(np.float32) | ||
|
|
||
| ref = np.arange(24).reshape((4, 6)).astype(np.float32) |
* use ``devito.tools.as_tuple`` to normalize the polymorphic ``axes`` of ``transpose`` (no-args, single ``None``, single tuple/list, or individual positional args all collapse to a flat tuple). * drop the ``isinstance(ret, Data)`` guards in ``transpose`` and ``swapaxes`` -- ``numpy.ndarray.<view-op>`` preserves the subclass via ``__array_finalize__``, so ``ret`` is always a ``Data``. * replace the hand-built reference in ``test_slice_after_transpose`` with ``ref = np.array(f.data)`` (and the 3D equivalent for ``g``), removing the duplicated ``arange/reshape/astype`` lines; shape and values are still compared against the NumPy reference array via ``np.array_equal``.
Contributor
|
Thank you, please rebase with commit messages according to guidlines and this is gtg |
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.
Fix: permute
Data._decomposition/_moduloon transposeFixes #2187.
Problem
A
Dataview created via.T,transpose(...)orswapaxes(...)inherited the source's
_decompositionand_modulotuples unchangedthrough
__array_finalize__(theself.ndim == obj.ndimbranchexplicitly does
self._modulo = obj._modulo,self._decomposition = obj._decomposition). Both tuples are indexed byaxis, so after the axes are permuted they point at the wrong
dimensions. A subsequent slice goes through
__getitem__→_index_glb_to_loc(glb_idx), which uses the (stale)_decompositionto translate the global slice into local-axis ranges, and silently
returns a wrong-shaped view.
The minimal example from #2187:
f.data[::2, ::2].T(slice then transpose) works because the slicegoes through
__getitem__first and the second-step transpose onlyflips a 2-element metadata tuple whose entries are already aligned.
Why both
transpose()andTneed overridingndarray.Tis a C-level shortcut: it does not dispatch throughPython's
transposemethod on a subclass. Verified directly:So overriding
transposealone leaves.Tbuggy; overridingTaloneleaves explicit
transpose/swapaxescalls buggy. Three thinoverrides are added on
Data:transpose(*axes)— normalizeaxes(no-arg, singletuple/list/iterable, or per-arg individual axes), call
super().transpose, then permute_decomposition/_modulointothe new axis order and refresh
_is_distributedso it staysconsistent.
swapaxes(axis1, axis2)— same treatment for the two-axis case.T— explicit Python property that forwards totranspose().Verification
The two new tests in
tests/test_data.py::TestDataBasic:test_slice_after_transpose— covers.T,transpose(),transpose((1, 0, 2)),swapaxes(0, 1), in both 2D and 3D, withthe NumPy reference array as the source of truth (shape and
values are compared).
test_transpose_permutes_data_metadata— asserts_decomposition/_moduloend up in the new order (reverse for.T, swap forswapaxes, explicit permutation fortranspose((1, 2, 0))).Local run on
mainHEAD (no MPI installed; MPI-only tests pre-existingfailures are unchanged):
Scope
Just
devito/data/data.py(production fix) andtests/test_data.py(regression tests). No public API change —
transpose/T/swapaxessignatures are unchanged; behavior matches NumPy.