refactor(pybind): bind in-place methods directly, drop the c__* shadow API#986
Conversation
…w API (#779) In-place dunders and _-suffixed methods are now bound under their real names with lambdas that take and return the same py::object, so python object identity is preserved and chaining works without the *_conti.py monkey-patch delegation layer. The c__iadd__/cConj_/... shadow bindings and their python wrappers are removed; genuinely python-side features (iterators, Network.Diagram, Qs, UniTensor.at proxy) remain. Also converts UniTensor's cInv_ (a shadow binding present in unitensor_py.cpp with no Python-side wrapper at all, so Inv_() was previously unreachable from Python) to Inv_, matching the pattern used for the rest of the in-place API. Storage's 11 c_pylist_<T> bindings collapse into one pylist() binding that switches on self.dtype() in C++. Bond's c_redirect_, c_getDegeneracy_refarg and c_group_duplicates_refarg fold their Python-side wrapper logic (the return_indices flag and the (result, mapper) tuple construction) directly into the C++ lambdas. tests/test_apply_pybind.py used c_redirect_() directly; updated to redirect_() to match. (Its 2 pre-existing TestApplyBlockFermionicUniTensor signflip failures are unrelated to this change -- reproduced at baseline before this commit.) Preserves two pre-existing quirks bug-for-bug rather than silently "fixing" them as drive-by changes: Tensor's c__ifloordiv__ called Div_ (not a real floor division), and UniTensor's ctruncate_(label, dim) string-overload called the non-mutating truncate() and discarded the result, making it a silent no-op on self (only the bond_idx overload actually mutates). Co-Authored-By: Claude <noreply@anthropic.com>
The string-label overload of UniTensor.truncate_ called the non-mutating
truncate() and discarded the result, so ut.truncate_("label", dim) was
a silent no-op (carried over bug-for-bug from the old ctruncate_ shadow
binding; present on master before the #779 refactor too). It now calls
the in-place C++ truncate_(label, dim), matching the bond_idx overload.
Adds regression pytests covering in-place mutation via a string label
and agreement between the string-label and bond-index overloads.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #986 +/- ##
==========================================
+ Coverage 30.87% 30.93% +0.06%
==========================================
Files 229 229
Lines 34757 34840 +83
Branches 14409 14383 -26
==========================================
+ Hits 10730 10777 +47
- Misses 16720 16752 +32
- Partials 7307 7311 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
How will this interact with #912? It seems |
|
@IvanaGyro good catch — they do touch the same surface, but it's a stub-vs-binding staleness rather than a hard code conflict. Both branches edit Concretely, what this PR does to that surface in
The interaction is that #912 ships generated, committed stubs, and its So once this PR lands, those Suggested ordering: land this PR first — it deletes the entire |
Code ReviewRemoves the Correctness — verified
Findings1. 2. 3. 4. 5. Verbosity (nit, acknowledged). The per-dtype overload duplication is large; already flagged as planned follow-up. VerdictStrong, careful refactor: the mechanical conversion is correct and complete, it deletes a real maintenance wart, fixes a genuine Posted by Claude Code on behalf of @pcchen |
pcchen
left a comment
pcchen
left a comment
There was a problem hiding this comment.
Approving. The refactor is correct and complete: the identity-preserving py::object self idiom is applied consistently, no shadow-name call sites dangle anywhere in the tree (verified against the head tree), no operator overloads were dropped, the truncate_(string, dim) C++ overload exists so commit 2's no-op fix is real, and full build/test/wheel CI is green on every platform. It also deletes a real maintenance wart and fixes a genuine silent no-op bug.
Non-blocking follow-ups (fine to do here or in a fast-follow):
- Add an
@=identity test + a changelog line.__imatmul__was previously misspelleddef __imatmulin the Python wrapper, soa @= bfell back toa = a @ b(new object); it now truly mutates in place and preserves identity. This is the one genuine behavioral change and is currently untested. - That same test (plus one numpy-scalar in-place op) would also clear the
codecov/patchgate.
Posted by Claude Code on behalf of @pcchen
| assert b.type() == cytnx.BD_BRA | ||
|
|
||
|
|
||
| def test_shadow_bindings_are_gone(): |
There was a problem hiding this comment.
This test is useless for the future code. This should be clean up.
There was a problem hiding this comment.
Agreed — it's a one-time migration guard with no value for future code (and the last reference to those dead c_* names). Since #986 is already merged, I've removed it in a follow-up: #1009. That PR also adds a real @= identity test in its place (the one genuine behavioral change from this refactor, which was untested), so the file keeps only the meaningful contract tests.
test(pybind): drop shadow-name guard, cover @= identity (follow-up to #986)
…nsor signflip Bond's public interface can no longer mutate a bond in place: a Bond bound into a UniTensor's block structure could previously be desynchronized via set_type/redirect_/combineBond_/clear_type, silently corrupting the derived _blocks/_inner_to_outer_idx (#846). All four are removed; the return-new forms (retype/redirect/combineBond) are the only way to change a bond's type or combine bonds, so sharing the PIMPL across copies is now safe. Non-const qnums()/syms() and the mutable getDegeneracies() overload are removed for the same reason, as is group_duplicates_() (the in-place counterpart of the kept group_duplicates()). UniTensor's non-const bonds() overload is removed too; internal code that legitimately replaces which bonds a tensor holds now goes through _bonds directly (already public on UniTensor_base) instead of mutating a shared Bond's impl. Bond::clone() deliberately remains a deep copy: #846's suggestion to collapse it to an identity copy is left as a follow-up, because the migrated combine paths now rely on clone-then-mutate of a detached impl (via the internal Bond_impl::force_combineBond_) as their safety argument. Internal call sites that relied on in-place bond mutation (Transpose_, combineBonds, group_basis_ in Dense/Block/BlockFermionicUniTensor; output bond retagging in Svd/Gesvd/Qr/Qdr/Rsvd and their truncate variants) now build new Bond values via retype()/redirect()/combineBond() and assign them back, or clone before calling the internal Bond_impl::force_combineBond_. Per #841, BlockFermionicUniTensor::_signflip moves from public to private. The public signflip_() mutable accessor (UniTensor_base virtual, the BlockFermionicUniTensor override, and the UniTensor wrapper) is deleted; the read-only signflip() stays. The SVD/Gesvd/QR/Eig/Eigh/ExpH/ExpM decomposition internals across src/linalg/*.cpp are the actual encapsulation unit that legitimately reads an input's signflip and initializes/truncates a freshly-built output's signflip in lockstep with its _blocks -- broader than #841's Svd_truncate.cpp-only enumeration, since Qr/Eig/Eigh/ExpH/ExpM/Rsvd construct BlockFermionicUniTensor by hand (never call Init()) and read/write _signflip the same way. They're granted access via a single friend function, linalg::_fermionic_signflip_(), rather than friending each internal helper (several are static/anonymous-namespace and overloaded, which would mean duplicating fragile forward declarations). pybind/bond_py.cpp drops the bindings for the removed mutators (redirect_ was bound by #986); Python users get the return-new forms only, hence the breaking-change marker on this commit. The python group_duplicates_ binding is also removed: it was a pre-existing misbinding of the OUT-of-place C++ Bond::group_duplicates under an in-place-mutator name (pybind treats the mapper output-reference argument as an input, so it never behaved as documented); the non-underscore group_duplicates binding that returns (Bond, mapper) remains. Tests: Bond_test.cpp pins that redirect()/combineBond()/retype() return new values while the receiver (and anything aliasing its impl) is untouched; BlockUniTensor_test.cpp adds a regression that a Bond obtained from a UniTensor can't desync its block structure; Dense/Block/BlockFermionic UniTensor tests add an aliasing regression pinning that a user-held bond copy is not rewritten by combineBond(force=true) (guards the .clone() that detaches the impl before the internal in-place combine); linalg_test.cpp adds a signflip/Nblocks lockstep check on BlockFermionicUniTensor post-truncation. Existing tests that called the removed mutators are rewritten to the return-new forms (including tests/test_apply_pybind.py, which sits outside the pytests/ CI path); pytests/binding_inplace_test.py replaces the in-place redirect_ identity test with a return-new test plus an AttributeError check for the five removed python methods. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 participants
Fixes #779, fixes #336.
What this does
In-place dunders and
_-suffixed methods were bound under shadow names (c__iadd__,cConj_,c_relabel_, …) and re-exported by monkey-patching wrappers incytnx/*_conti.pythat called the shadow binding and returnedself. That left three inconsistent return conventions, a wasted Python-object copy on every in-place op, and the wrong API surface inhelp()/stubs.Commit 1 (
efa8ea43) binds the real method names directly with lambdas that take and return the samepy::object, so Python object identity is preserved and chaining (t.Conj_().Abs_(),ut.set_name("x").relabel_(["a","b"])) works without the delegation layer:tensor_py.cpp:__iadd__/__isub__/__imul__/__itruediv__/__ifloordiv__/__ipow__/__imatmul__,Conj_/Exp_/InvM_/Inv_/Abs_/Pow_unitensor_py.cpp:Conj_/Trace_/Transpose_/normalize_/Dagger_/tag/__ipow__/Pow_/truncate_/set_name/set_label(s)/relabel_/relabels_/set_rowrank_/convert_frombond_py.cpp:redirect_;getDegeneracy(qnum, return_indices=)andgroup_duplicates()now return their aux indices directly from C++storage_py.cpp: the 11c_pylist_<T>bindings collapse into onepylist()that switches onself.dtype()c__*/c*shadow bindings and their*_conti.pywrappers are removed; genuinely Python-side features (iterators,Network.Diagram,Qs, theUniTensor.atproxy) remain.Commit 2 (
a3367f99) fixes a bug this conversion surfaced: the string-label overload ofUniTensor.truncate_called the non-mutatingtruncate()and discarded the result, sout.truncate_("label", dim)was a silent no-op. This is pre-existing on master (the oldctruncate_shadow binding had the same call), not introduced by the refactor. It now calls the in-place C++truncate_(label, dim), matching thebond_idxoverload.Testing
pytests/binding_inplace_test.py: object-identity (r is t) and chaining guarantees,Storage.pylistround-trip,Bond.group_duplicatestuple return, and a grep-style guard that no shadow names survive on the Python classes.pytests/unitensor_test.pyfor thetruncate_fix: string-label truncation actually shrinks the bond in place (was red before commit 2 — shape stayed[3, 4]), and the string-label/bond-index overloads agree.Notes for reviewers
r is tisTrue); previously some paths returned a fresh wrapper around the same C++ impl.ut.truncate_("label", dim)now actually truncates — code that accidentally relied on the no-op will see the bond shrink.__ne__/__bool__) is planned as separate PRs stacking on this one.🤖 Generated with Claude Code