[Bug] Fix alternating qd.Tensor wrapper unwrapping - #863
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08d57644c8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| _indices = tuple( | ||
| i | ||
| for i, (a, m) in enumerate(zip(py_args, self.arg_metas)) | ||
| if type(a) in _TENSOR_WRAPPER_TYPES or m.annotation is _TensorClass |
There was a problem hiding this comment.
Document alternating qd.Tensor argument calls
This line makes qd.Tensor-annotated parameters persist as unwrap candidates even when earlier launches used None or a bare impl, and the added tests pin that optional/bare/wrapper alternation as supported user-facing call behavior. The diff only updates code and tests, so docs/source/user_guide/tensor.md still documents qd.Tensor parameters as receiving tensor values without explaining the new optional/bare alternation pattern or the need to guard None with qd.static.
AGENTS.md reference: AGENTS.md:L15-L22
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks, good call. I documented that qd.Tensor arguments may alternate among wrappers, bare field or ndarray implementations, and None, including the qd.static guard for the None case. The focused Metal selection remains green (4 passed), and the Markdown link, non-ASCII, and pre-commit checks pass.
Issue: #856
Brief Summary
Kernel.__call__caches wrapper positions on the first launch and assumes those positions always hold aqd.Tensorwrapper. A laterNoneor bare field/ndarray implementation can therefore fail while accessing._impl, while a wrapper first seen after an unwrapped value can bypass canonicalization. Cache exactqd.Tensorannotation positions as candidates and unwrap only exact Tensor wrapper types. Canonicalization remains before autodiff recording, template mapping, and AST construction.This correction is separate from #859. It does not change static identity checks, fastcache serialization, accepted annotations, or kernel defaults.
Walkthrough
qd.Tensorannotations while retaining wrappers found in other slots on the first launch.._impl, so wrappers,None, and bare implementations may alternate.None, wrapper-to-bare,None-to-wrapper Tape replay, and mapper identity stability.Nonecalls forqd.Tensorparameters.Validation: the four focused cases pass on Metal; the same cases produce four causal failures on exact base
ab9a58ab5. The compatible tensor-annotation selection reports 12 passed, 9 skipped, and 4 deselected; adjacent kernel, template, and cache tests report 38 passed and 8 skipped; adjacent autodiff tests report 89 passed and 9 skipped; full pre-commit passes. CPU testing is unavailable on this host because both the exact base and this commit hit the existing LLVMUnsupported stack probing methodfailure before the focused assertions.Performance: A controlled, interleaved Metal launch microbenchmark found overlapping retained fresh-process median ranges on the changed launch path. The one-wrapper ranges were 11.78 to 11.90 microseconds for this commit and 11.77 to 11.85 microseconds for the base; the eight-wrapper ranges were 13.56 to 13.73 and 13.55 to 13.73 microseconds. On Genesis
7fb4614's anymal workload, an instrumented same-scene Metal/ndarray crossover swapped only the exactKernel.__call__method after fixed untimed cache settlement. The candidate-over-base effect was +1.6082369%; the two-sided t(15) 95% CI was -0.2411585% to +3.4919176%. Across 16 predeclared, counterbalancedABBA|BAABsuperblocks, all 128/128 timed windows were clean, covering 524,288 timed steps with no exclusions or reruns. The lower endpoint clears the predeclared -0.50% non-inferiority margin. This is model-based evidence for this scene and host, not an official Genesis benchmark or evidence of a speedup. The unmodified benchmark cannot initialize a display context on this headless macOS host, so the run used only a display-only in-memoryRasterizer.buildbypass; physics, stepping, control, benchmark timing logic, and tracked source were unchanged.