Conversation
… need to be careful when dealing with fermions
… &mapper) by copying and adapting the updated cytnx_uint64 version
… legs of type BD_KET ("P gate")
…ion, also for bosonic cases
-twist_ implemented to be called by label as well
…owrank accordingly to really transpose the matrix form of a tensor
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #703 +/- ##
==========================================
+ Coverage 35.65% 36.23% +0.58%
==========================================
Files 214 214
Lines 33068 33132 +64
Branches 13157 13171 +14
==========================================
+ Hits 11789 12005 +216
+ Misses 19349 19184 -165
- Partials 1930 1943 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Is it possible to add some simple unit test? |
done |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba15358496
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1) Transpose sets is_diag correctly after changing rowrank 2) Error messages print unknown labels as strings instead of integers
|
All fixed. |
pcchen
left a comment
pcchen
left a comment
There was a problem hiding this comment.
PR Review: Fermion twist
Posted by Claude Code on behalf of @pcchen
Summary
This PR adds fermion twist operations (twist, twist_, fermion_twists, fermion_twists_) to UniTensor, implementing the P-gate concept from arXiv:2404.14611. It also makes several methods return *this for method chaining, improves error messages and documentation, and capitalizes cannot → Cannot throughout the codebase.
The core logic is reasonable. There are a few real bugs to address, along with some concerns about the scope of the change.
Critical Issues
1. Debug cout left in BlockFermionicUniTensor::permute_nosignflip_
src/BlockFermionicUniTensor.cpp, permute_nosignflip (string-label overload):
mapper_i64.push_back(std::distance(out_raw->_labels.begin(), it));
}
cout << "Mapper: " << mapper_i64 << endl; // <-- debug output in production!This will print to stdout on every call to permute_nosignflip by label. Should be removed.
2. No tests for the new API
The PR adds twist(), twist_(), fermion_twists(), fermion_twists_() (C++ and Python), but no test files are modified. These operations are non-trivial (they flip sign bits on specific blocks), so correctness should be verified by unit tests covering at minimum:
twist_on a single-bond fermionic tensor with known parityfermion_twists_on a multi-bond tensor- Calling
twiston a bosonic tensor (should be a no-op) - Invalid index / label error paths
Important Issues
3. UniTensor_base::twist_() base implementations always throw
src/UniTensor_base.cpp:
void UniTensor_base::twist_(const cytnx_int64 &idx) {
cytnx_error_msg(
true, "[ERROR] fatal internal, cannot call on an un-initialized UniTensor_base%s", "\n");
}These always throw, with an error message about "un-initialized UniTensor_base". In practice, concrete types (DenseUniTensor, BlockUniTensor, BlockFermionicUniTensor) all override them correctly, so this is never reached for initialized tensors. However, the error message is wrong — it says "un-initialized UniTensor_base" but it would be triggered for any type that inherits without overriding (e.g., VoidUniTensor or future subclasses). Consider making these pure virtual or at least fixing the error message.
4. fermion_twists_() docstring is contradictory
include/UniTensor.hpp, docstring for fermion_twists():
@brief Apply twists to all bra bonds with type BD_KET
"Bra bonds with type BD_KET" is self-contradictory. Bra bonds are typically BD_BRA. The implementation applies twists to bonds at positions idx >= rowrank that have type BD_KET. The docstring needs to be reworded to clearly describe this: e.g., "Apply twists to all bonds in bra-position (index ≥ rowrank) that have bond type BD_KET."
5. twist_(std::string label) should be const std::string &
include/UniTensor.hpp and src/BlockFermionicUniTensor.cpp:
virtual void twist_(const std::string label); // base class
void twist_(const std::string label) override { ... } // DenseUniTensor, BlockUniTensor
void BlockFermionicUniTensor::twist_(const std::string label) { ... }All overloads take std::string by value. Throughout the codebase, label parameters are consistently passed as const std::string &. This should be const std::string &label to match conventions and avoid unnecessary copies.
Minor Issues
6. Typo in docstring: "weather" → "whether"
include/UniTensor.hpp, twist() docstring:
@note This always applies the twist to the bond, ignoring its direction or weather they are
incoming or outgoing bonds.
Should be "whether".
7. Duplicate sentence in get_block warning
include/UniTensor.hpp, warning added to get_block(const cytnx_uint64 &idx = 0):
Use signflip() to get the sign structure for each block. Use signflip() to get the sign
structure for each block.
The sentence "Use signflip() to get the sign structure for each block." appears twice.
8. Commented-out code in include/UniTensor.hpp
A block of commented-out permute_ overloads was added:
// void permute_( const std::initializer_list<char*> &mapper, ...
// void permute_(const std::vector<cytnx_int64> &mapper, ...These should be removed if no longer needed.
9. Unsigned underflow in twist_ error message
src/BlockFermionicUniTensor.cpp, twist_(const cytnx_int64 &idx):
cytnx_error_msg(idx >= this->_labels.size() || idx < 0,
"...[0, %d].\n",
idx, this->_labels.size() - 1);When _labels.size() == 0, _labels.size() - 1 underflows (wraps to a very large unsigned number). Protect with a check or use a signed cast.
10. Scope of capitalization churn
The cannot → Cannot change spans ~200 locations across Scalar.hpp, Storage_base.cpp, StorageImplementation.cpp, MPS.hpp, SparseUniTensor.cpp, RegularGncon.cpp, and many more. While consistency is good, mixing this into a feature PR makes it substantially harder to review. Consider isolating it in a separate PR.
Positive Observations
- The design is clean: bosonic types override
twist_()as a no-op, and onlyBlockFermionicUniTensorhas the real implementation. - Method chaining (returning
*this) is a useful ergonomic improvement. - The
fermion_twists_()documentation correctly describes the physical motivation (scalar products of fermionic states). - Error messages for
permute,permute_nosignflip, andcontractare now significantly more informative with tensor names and label values included. - Debug
couts removed fromcontractare appropriate cleanup. - The
Transpose_()rowrank expression simplification is correct.
Recommended Actions
- Remove the debug
coutinpermute_nosignflip(item 1) — will spam stdout in production. - Add unit tests for
twist_()andfermion_twists_()(item 2). - Fix docstring contradiction in
fermion_twists()(item 4). - Pass label by
const std::string &intwist_()overloads (item 5). - Fix the "weather" typo (item 6) and duplicate sentence (item 7).
- Consider splitting the capitalization changes into a separate PR (item 10).
pass string by reference debug cout removed and cleanup docstring/error message updates
|
Use master's build_accessors() helper in __getitem__ and __setitem__
instead of the inlined accessor-building logic from fermion_twist.
Also picks up master's typo fix ("Blcok" -> "Block") in error message.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f298349f7e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .def("permute_nosignflip_", [](UniTensor &self, const std::vector<cytnx_int64> &mapper, const cytnx_int64 &rowrank){ | ||
| self.permute_nosignflip_(mapper,rowrank); | ||
| return self.permute_nosignflip_(mapper,rowrank); |
There was a problem hiding this comment.
These new in-place bindings return from lambdas without an explicit return type, so C++ deduces auto as UniTensor (by value) rather than UniTensor&. That means self.permute_nosignflip_(...)/self.twist_(...) mutate self but then return a copy, so chained calls execute on a temporary copy instead of the original object unless the caller reassigns (e.g. u.permute_nosignflip_(...).twist_()). Use an explicit -> UniTensor& return type (and reference return policy) for these overload lambdas.
Useful? React with 👍 / 👎.
4 participants
-Added fermion twists, which corresponds to the P-gates in arXiv:2404.14611
-Cleaned up code and updated documentation
-Made all methods return a pointer to the class to allow for chaining (e.g.
UniTensor.contiguous().fermion_twists(). ...)