Conversation
…(possibly with bonds removed)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #714 +/- ##
==========================================
+ Coverage 32.35% 35.32% +2.97%
==========================================
Files 215 215
Lines 36363 33133 -3230
Branches 14597 13215 -1382
==========================================
- Hits 11764 11705 -59
+ Misses 22659 19509 -3150
+ Partials 1940 1919 -21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Is it possible to add some simple unit test? |
manuschneider
force-pushed
the
slicing
branch
from
e65f904 to
25121c9
Compare
|
@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: 25121c9397
ℹ️ 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".
| */ | ||
| Tensor get(const std::vector<cytnx::Accessor> &accessors) const { | ||
| Tensor get(const std::vector<cytnx::Accessor> &accessors, | ||
| std::vector<cytnx_int64> &removed = *new std::vector<cytnx_int64>()) const { |
There was a problem hiding this comment.
Tensor::get now defaults removed to *new std::vector<cytnx_int64>(), so every call site that omits this argument leaks one heap allocation; this includes common slicing paths where get is called repeatedly. Because the pointer is never owned or freed, long-running workloads will steadily grow memory usage. Please replace this with an overload (or another non-owning optional pattern) instead of allocating in the default argument.
Useful? React with 👍 / 👎.
| cytnx_error_msg(Args.size() > 2, | ||
| "[ERROR][slicing] A diagonal UniTensor can only be accessed with one- or two dimensional slicing.%s", "\n"); |
There was a problem hiding this comment.
This branch now accepts 2D tuple indexing for diagonal __setitem__, but DenseUniTensor::set still forwards directly to the rank-1 diagonal block tensor, so two accessors trigger the Tensor rank check (accessors.size() > _shape.size()) and fail at runtime. In practice, assignments like diagonal ut[1, :] = ... are now parsed as valid here but cannot succeed in the backend (same issue exists in the UniTensor rhs overload below).
Useful? React with 👍 / 👎.
| boost::intrusive_ptr<UniTensor_base> out(new DenseUniTensor()); | ||
| out->Init_by_Tensor(this->_block.get(accessors), false, 0); // wrapping around. | ||
| return out; | ||
| if (accessors.empty()) return this; |
There was a problem hiding this comment.
The new early return gives back this for an empty accessor list, which makes UniTensor::get({}) alias the original object instead of producing an independent result. That contradicts the documented get contract (no shared memory) and can cause unexpected mutation coupling when callers modify the returned tensor. This also becomes reachable from Python diagonal indexing with an empty tuple.
Useful? React with 👍 / 👎.
3 participants
UniTensors keep their metadata (possibly with bonds removed for constant access of one element in a given dimension).
Diagonal UniTensors can be accessed by two-dimensional slicing as well now.
This fixes #397