changed get() and slicing by manuschneider · Pull Request #714 · Cytnx-dev/Cytnx

manuschneider · 2025-11-24T18:02:33Z

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

…(possibly with bonds removed)

codecov · 2025-11-24T18:22:13Z

Codecov Report

❌ Patch coverage is 61.45833% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 35.65%. Comparing base (397ecbc) to head (b527d89).
⚠️ Report is 67 commits behind head on master.

Files with missing lines	Patch %	Lines
include/UniTensor.hpp	23.33%	21 Missing and 2 partials ⚠️
src/DenseUniTensor.cpp	66.66%	7 Missing and 7 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #714      +/-   ##
==========================================
+ Coverage   35.44%   35.65%   +0.20%     
==========================================
  Files         215      214       -1     
  Lines       33071    33068       -3     
  Branches    13170    13157      -13     
==========================================
+ Hits        11723    11789      +66     
+ Misses      19424    19349      -75     
- Partials     1924     1930       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…dated doc

hunghaoti · 2025-11-27T05:11:46Z

Is it possible to add some simple unit test?

ianmccul · 2026-03-18T14:45:50Z

@codex review

chatgpt-codex-connector

💡 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".

set behaves like get for diagonal UniTenosrs get always returns a UniTensor with independent metadata no memory leak with empty 'removed' argument using get()

…th two accessor elements on a diagonal UniTensor this creates consistency; use one accessor element to address only the diagonal elements;

manuschneider · 2026-03-30T16:17:28Z

All issues were fixed, and unit tests were added.

I changed the behavior so that slicing a diagonal-dense UniTensor with two accessors always yields a non-diagonal UniTensor. Before, the result was still diagonal if the two accessors were the same, but this led to inconsistent behavior. If the output should be diagonal, use only one accessor!

pcchen

PR Review: changed get() and slicing

Posted by Claude Code on behalf of @pcchen

Summary

This PR extends get() and slicing for UniTensor:

Diagonal UniTensors can now be sliced with two-dimensional accessors (converting to dense)
UniTensor::operator[] overloads are added
Accessor::operator== is added
Bond metadata (labels, rowrank) is now preserved through slicing

The approach is sound and well-tested. There are a few bugs and style issues to address before merging.

Critical Issues

1. Memory leak in `DenseUniTensor::get()` — diagonal two-accessor path

src/DenseUniTensor.cpp, in the else branch for 2 accessors:

DenseUniTensor *out = this->clone_meta();  // allocates raw pointer
...
} else {  // convert to non-diagonal UniTensor
  out->_block = this->_block;
  out->to_dense_();
  return out->get(accessors);  // 'out' is NEVER freed — memory leak
}

out->get(accessors) returns a new boost::intrusive_ptr<UniTensor_base> wrapping a different object. The out raw pointer is never placed in an intrusive_ptr, so it leaks on this path.

Fix: wrap out before the recursive call:

boost::intrusive_ptr<DenseUniTensor> out_ptr(out);
out_ptr->_block = this->_block;
out_ptr->to_dense_();
return out_ptr->get(accessors);

2. Wrong error tag in `DenseUniTensor::set()`

src/DenseUniTensor.cpp, set() method, two-accessor diagonal path:

"[ERROR][DenseUniTensor][get] For diagonal UniTensors, only one or two "

The tag says [get] but this is the set() function. Should be [set].

Important Issues

3. Type inconsistency: `cytnx_int32` vs `cytnx_int64` for `removed`

src/backend/Tensor_impl.cpp, set() path:

-      std::vector<cytnx_int32> remove_id;
+      std::vector<cytnx_int32> removed;   // still int32!

The rename was done but the type stayed cytnx_int32, inconsistent with get() which uses cytnx_int64. While this is a local variable with no external impact, it should be consistent.

4. `_is_braket_form` update is one-sided

src/DenseUniTensor.cpp, non-diagonal path:

if (out->is_tag() && !out->_is_braket_form) {
  out->_is_braket_form = out->_update_braket();
}

This only promotes _is_braket_form from false to true, never the reverse. Slicing can also invalidate a previously valid braket form (e.g., if bonds are removed unevenly). Should be:

if (out->is_tag()) {
  out->_is_braket_form = out->_update_braket();
}

Minor Issues

5. Commented-out dead code in `pybind/unitensor_py.cpp`

The old error path is left as commented code in three places:

// cytnx_error_msg(true,
// "[ERROR] cannot get element using [tuple] on is_diag=True UniTensor since the block is rank-1, consider [int] or [int:int] instead.%s", "\n");

Please remove these.

6. Unused variable `cnt` in `pybind/unitensor_py.cpp`

In all three copies of the new diagonal slicing code block:

cytnx_uint64 cnt = 0;
for (...) {
    cnt++;  // incremented but never read

cnt should be removed.

7. Triplicated code in `pybind/unitensor_py.cpp`

The diagonal two-accessor handling block is copy-pasted verbatim three times (for __getitem__, a second __getitem__ variant, and __setitem__). Consider extracting a helper lambda or function to reduce maintenance burden.

Positive Observations

The removed out-parameter approach for propagating squeezed dimensions from Tensor_impl up to DenseUniTensor is clean and doesn't break the public API.
Test coverage is thorough: single/double accessor cases, diagonal/non-diagonal, get/set/operator[], and immutability of source.
The AreEqUniTensorMeta helper is a useful addition.
Fixing get/set_block → get/put_block in error messages is a good cleanup.
The Accessor::operator== implementation is correct and well-tested.

Recommended Actions

Fix the memory leak (item 1) — critical, causes heap corruption in the diagonal two-accessor get() path.
Fix the wrong [get] tag in set() (item 2).
Fix _is_braket_form update (item 4).
Remove commented-out code and unused cnt (items 5–6).
Consider extracting the duplicated pybind diagonal slice code (item 7).

DenseUniTensor.cpp: Pointer in get() freed before return DenseUniTensor.cpp: Type consistency for varibable `removed` unitensor_py.cpp: Using Lambda to avoid code repetition for bindings __setitem__ and __getitem__ Typos fixed, cleanup, removed unused variable cnt

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b527d89b17

ℹ️ 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".

chatgpt-codex-connector · 2026-04-11T13:38:42Z

  }
+
+  boost::intrusive_ptr<UniTensor_base> DenseUniTensor::get(const std::vector<Accessor> &accessors) {
+    if (accessors.empty()) return this->clone_meta();


Populate block when returning get({}) clone

Even though aliasing was removed, this new get fast path returns clone_meta() without copying _block, so UniTensor::get({}) can produce a tensor with metadata but no backing data. That makes subsequent reads inconsistent (and diagonal shape/data access can fail because _is_diag is preserved while _block is empty), and Python diagonal ut[()] can hit this path via the new accessor builder.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-04-11T13:38:43Z

+  }
+
+  void DenseUniTensor::set(const std::vector<Accessor> &accessors, const Tensor &rhs) {
+    if (accessors.empty()) return;


Apply empty-accessor assignments instead of returning early

This early return makes DenseUniTensor::set silently ignore set({}, rhs), which is a behavior regression from forwarding to Tensor::set (where an empty accessor list means full-slice assignment after axis fill). With the new Python accessor logic, diagonal ut[()] = rhs can now become a no-op rather than updating data.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-04-11T13:38:43Z

+        for (unsigned int j = 0; j < removed.size(); j++) {
+          if (new_mapper[i] > removed[j])


Exclude stale removed indices from get() permutation

Tensor_impl::get(accessors, removed) now uses the caller-provided removed vector directly when building perm, so any pre-existing entries in that output vector influence the returned tensor layout. Since this API is documented to append removed axes, callers can reasonably reuse a non-empty vector across calls, and stale indices will incorrectly drop/shift dimensions in unrelated slices.

Useful? React with 👍 / 👎.

manuschneider · 2026-04-11T13:39:39Z

1. fixed, create intrusive_ptr before return
2. fixed
3. fixed
~~[ ]~~ 4. This does not need to be fixed: a tensor in braket form can never become a non-braket by slicing, even if bonds are removed.
5. fixed
6. fixed
7. fixed, using Lambda now to avoid repetitive code

changed get() and slicing such that UniTensors remain their metadata …

ce532a4

…(possibly with bonds removed)

fixed get() for diagonal UniTensor, added operator[] to UniTensor, up…

7cc7ba7

…dated doc

manuschneider added 2 commits February 23, 2026 17:13

Merge remote-tracking branch 'master' into slicing

4a7b337

added unit tests for slicing of DenseUniTensors

25121c9

manuschneider force-pushed the slicing branch from e65f904 to 25121c9 Compare February 23, 2026 17:06

manuschneider added the Pending check/approval Issue fixed, and need feedback label Feb 24, 2026

chatgpt-codex-connector bot reviewed Mar 18, 2026

View reviewed changes

Comment thread include/Tensor.hpp Outdated

Comment thread pybind/unitensor_py.cpp Outdated

Comment thread include/UniTensor.hpp Outdated

manuschneider added 4 commits March 30, 2026 23:03

fixed issues:

82d737f

set behaves like get for diagonal UniTenosrs get always returns a UniTensor with independent metadata no memory leak with empty 'removed' argument using get()

get/set always create a non-diagnal UniTensor when they are called wi…

a11a4b9

…th two accessor elements on a diagonal UniTensor this creates consistency; use one accessor element to address only the diagonal elements;

added unit tests

e53fc9b

Merge remote-tracking branch 'master' into slicing

5b89af3

pcchen reviewed Apr 11, 2026

View reviewed changes

Fixed issues

b527d89

DenseUniTensor.cpp: Pointer in get() freed before return DenseUniTensor.cpp: Type consistency for varibable `removed` unitensor_py.cpp: Using Lambda to avoid code repetition for bindings __setitem__ and __getitem__ Typos fixed, cleanup, removed unused variable cnt

chatgpt-codex-connector bot reviewed Apr 11, 2026

View reviewed changes

pcchen approved these changes Apr 12, 2026

View reviewed changes

manuschneider merged commit 1acdacf into master Apr 12, 2026
7 checks passed

manuschneider deleted the slicing branch April 12, 2026 05:16

		for (unsigned int j = 0; j < removed.size(); j++) {
		if (new_mapper[i] > removed[j])

Conversation

manuschneider commented Nov 24, 2025

Uh oh!

codecov bot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hunghaoti commented Nov 27, 2025

Uh oh!

ianmccul commented Mar 18, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

manuschneider commented Mar 30, 2026

Uh oh!

pcchen left a comment

Choose a reason for hiding this comment

PR Review: changed get() and slicing

Summary

Critical Issues

1. Memory leak in DenseUniTensor::get() — diagonal two-accessor path

2. Wrong error tag in DenseUniTensor::set()

Important Issues

3. Type inconsistency: cytnx_int32 vs cytnx_int64 for removed

4. _is_braket_form update is one-sided

Minor Issues

5. Commented-out dead code in pybind/unitensor_py.cpp

6. Unused variable cnt in pybind/unitensor_py.cpp

7. Triplicated code in pybind/unitensor_py.cpp

Positive Observations

Recommended Actions

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Apr 11, 2026

Choose a reason for hiding this comment

Uh oh!

manuschneider commented Apr 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Nov 24, 2025 •

edited

Loading

1. Memory leak in `DenseUniTensor::get()` — diagonal two-accessor path

2. Wrong error tag in `DenseUniTensor::set()`

3. Type inconsistency: `cytnx_int32` vs `cytnx_int64` for `removed`

4. `_is_braket_form` update is one-sided

5. Commented-out dead code in `pybind/unitensor_py.cpp`

6. Unused variable `cnt` in `pybind/unitensor_py.cpp`

7. Triplicated code in `pybind/unitensor_py.cpp`