Repository navigation
fix: slice batch_prob per mask instead of indexing it - #92
Closed
Hendrik-code wants to merge 3 commits into
Closed
Hendrik-code wants to merge 3 commits into
Hendrik-code wants to merge 3 commits into
Conversation
`MaskSequentialOpsCustom.transform_list` set `params_i["batch_prob"] = params["batch_prob"][i]`, which is a 0-dim tensor. Kornia derives `to_apply = batch_prob > 0.5` and branches on `to_apply.all()` / `.any()`; for a 0-dim tensor those are always equal, so the partial branch is dead, and `in_tensor[to_apply]` would index along the wrong axis rather than select rows. Latent today: `AugmentationSequentialOpsCustom.transform` asserts the mask is a Tensor before the list branch can be reached, so nothing in SmaugLab gets here. The assert is the only thing standing between the bug and a caller who passes a list of masks, which is what the signature exists for. The test pins the contract and, for the loop itself, asserts on the source -- worth more than a test that cannot run the code, and it says why in the docstring. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The defect
MaskSequentialOpsCustom.transform_listbuilds per-mask parameters withparams["batch_prob"]is[B], so[i]yields a 0-dim tensor. Kornia then doesto_apply = batch_prob > 0.5and branches onto_apply.all()/.any()— which for a 0-dim tensor are always the same value, so the "some samples apply" branch is unreachable. Worse,in_tensor[to_apply]with a 0-dim boolean indexes along the wrong axis instead of selecting rows.Both per-mask loops in that method had it.
Honest scope
This is latent.
AugmentationSequentialOpsCustom.transformasserts the mask is aTensorbefore the list branch is reached, so nothing in SmaugLab can currently get here — I confirmed that by passing a list of masks through the pipeline and hitting the assert, not the loop. The slice is still wrong, and that assert is the only thing between it and a caller who uses the list signature this method exists to serve.The fix
params["batch_prob"][i : i + 1], which keeps the batch axis.Tests
unit_tests/test_mask_list_batch_prob.pypins the contract (a 1-element slice keeps the batch axis; a scalar index drops it, and.all()/.any()then coincide) and, for the loop itself, asserts on the source text. A test that cannot execute the code would be worth less; the docstring says exactly why it is written this way.Verified failing before the fix and passing after. Full suite unchanged against baseline;
ruff check .clean.🤖 Generated with Claude Code