fix: consistent shape convention in flip/reshape/broadcast_to/stack#149
Draft
henryiii wants to merge 1 commit into
Draft
fix: consistent shape convention in flip/reshape/broadcast_to/stack#149henryiii wants to merge 1 commit into
henryiii wants to merge 1 commit into
Conversation
flip, reshape, broadcast_to, and stack had numpy fast paths that produced fixed integer inner dimensions for uniform inputs, while their general (ragged) paths produced variable-length (None) inner dimensions. The same logical operation therefore returned a different shape signature depending on whether the data happened to be uniform — a silent behavioral fork. - flip/reshape: route the numpy fast path through _ak_from_numpy so inner dimensions come back variable-length, matching permute_dims/roll. - broadcast_to/stack: drop the numpy fast path entirely; the awkward general paths already produce the correct, consistent convention (and, for stack, correctly keep the newly inserted axis regular). meshgrid and the creation functions (zeros, ones, ...) are intentionally left unchanged: they create new regular data rather than transforming an existing ragged array. Assisted-by: ClaudeCode:claude-opus-4-8
27 tasks
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.
🤖 AI text below 🤖
Part of #144.
The inconsistency
ragged.array.shapeusesNonefor ragged (variable-length) dimensions and ints for regular dimensions. Several manipulation functions have a numpy "fast path" for uniform inputs and a general "ragged path" for everything else. For four of them, the two paths disagreed on the shape signature: the fast path made every inner dimension a fixed int, while the general path left the source array's own inner dimensions variable-length (None).This meant the same logical operation returned a different shape signature depending only on whether the data happened to be uniform — a silent behavioral fork. (
permute_dimsandrollwere already correct: they route their fast path through the existing_ak_from_numpyhelper.)For
x = ragged.array([[1.0, 2.0], [3.0, 4.0]])withx.shape == (2, None):flip(x, axis=1)(2, 2)(2, None)(2, None)flip(x)(2, 2)(2, None)(2, None)reshape(x, (2, 2))(2, 2)(2, None)reshape(x, (2, 2, 1))(2, 2, 1)(2, None, None)broadcast_to(x, (2, 2))(2, 2)(2, None)(2, None)stack([x, x])(2, 2, 2)(2, 2, None)(2, 2, None)stack([x, x], axis=2)(2, 2, 2)(2, None, 2)(2, None, 2)1-D inputs are unaffected (they have no inner ragged dimension):
reshape(v, (3,)),flip(v),broadcast_to(v, (2, 3)) -> (2, None),stack([v, v]) -> (2, 3).Approach per function
I verified the general path's output per function rather than assuming a single fix:
flip— route the numpy fast path through_ak_from_numpyinstead of plainak.from_numpy.flippreserves the input shape, so a(2, None)input must stay(2, None).reshape— same: route the multi-dimensional fast path through_ak_from_numpy. The reshaped result of a ragged-typed array is itself a transformed ragged array, so inner dims becomeNone. (The ragged path only ever produces 1-D output, so there is no ragged equivalent for multi-dim reshape to diff against; the convention matches the rest of the library.)broadcast_to—_ak_from_numpyalone is not correct here, because the general path keeps newly prepended outer dims regular while making the source's own inner dimsNone(e.g.(2, 3)-uniform →(4, 2, 3)target →(4, 2, None)). I confirmed the awkwardbroadcast_arraysgeneral path already produces exactly this convention for every test case (values identical), so I removed the numpy fast path and let the general path run unconditionally.stack— I ran both paths and compared. The general path (expand_dims+concat) keeps the newly inserted stack axis regular while making the source dimsNone(axis 0 →(2, 2, None), axis 2 →(2, None, 2)). Plain_ak_from_numpywould wrongly make all inner dimsNone((2, None, None)), so I removed the numpy fast path and let theexpand_dims+concatgeneral path run unconditionally. Added a parametrized test asserting the uniform and genuinely-ragged inputs produce the same shape structure for every axis.Boundary: what is intentionally not changed
meshgridand the creation functions (zeros,ones, etc.) are left unchanged. They create new regular data (e.g.ragged.zeros((2, 2))legitimately has int dims) rather than transforming an existing ragged array, so the int-dim convention is correct for them.Existing tests changed (user-visible behavior change)
These assertions pinned the old int-shape convention and were updated (with an explanatory comment in each):
tests/test_broadcast_to.py::TestBroadcastTo2DUniform::test_outer_replication—(4, 2, 3)→(4, 2, None)(2-D source's inner dim is variable-length).tests/test_broadcast_to.py::TestBroadcastTo3DUniform::test_outer_replication—(5, 2, 3, 4)→(5, 2, None, None)(3-D source is(2, None, None)).tests/test_reshape.py::TestReshapeRegular::test_add_trailing_dim—(2, 2, 1)→(2, None, None).The
stackshape tests already asserted the(2, 2, None)convention (those were the ragged-path tests), so no stack assertions needed changing — the uniform fast path now simply agrees with them.New tests pinning the convention
Added a
…ShapeConventionclass to each oftest_flip.py,test_reshape.py,test_broadcast_to.py, andtest_stack.pyasserting that a uniform-but-ragged-typed input produces the same shape signature as the general/ragged path (including a parametrizedstacktest that diffs the uniform vs genuinely-ragged result for every axis).Heads-up for maintainers
This is a user-visible behavior change to the returned
shapeofflip,reshape,broadcast_to, andstackon multi-dimensional inputs (values are unchanged; only the int-vs-Nonedimension signature changes). It is opened as a draft for discussion. One judgment call worth flagging: forbroadcast_towith an explicit all-int target shape, one could argue the result should be regular; I chose to match the ragged path (inner source dims →None) for consistency, since the same operation on a genuinely ragged source can only produceNonethere anyway.Full suite:
1313 passed, 6 skipped(pre-existing numpy-v1-only skips).prek -a(ruff + ruff-format + mypy --strict) clean.🤖 Generated with Claude Code