[codex] fix Tensor.Fromfile and Eig values-only regressions#884
[codex] fix Tensor.Fromfile and Eig values-only regressions#884ianmccul wants to merge 5 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several unit tests for Fromfile storage operations, CMake package configuration exports, and the linalg::Eig function. The review feedback highlights a critical bug in the underlying Storage::Fromfile implementation where an uninitialized variable Nelem causes undefined behavior when count >= 0. Additionally, the reviewer recommends extending the CMake test to build the consumer project rather than just configuring it, and refactoring the eigenvalue test to compare values in an order-independent manner to prevent flaky test failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| Storage source = Storage::from_vector(std::vector<cytnx_double>{1.0, 2.0, 3.0, 4.0}); | ||
| source.Tofile(path); | ||
|
|
||
| EXPECT_THROW(Storage::Fromfile(path, Type.Double, 5), std::logic_error); |
There was a problem hiding this comment.
Note that Storage::Fromfile currently contains a critical bug where the Nelem variable is used uninitialized when count >= 0 (which is the case here since count = 5).
Specifically, in src/backend/Storage.cpp:
199: cytnx_uint64 Nelem;
...
217: if (count < 0)
218: Nelem = Nbytes / Type.typeSize(dtype);
219: else {
220: cytnx_error_msg(count > Nelem, "[ERROR] count exceed the total # of elements %d in file.\n",
221: Nelem);When count >= 0, the else branch is taken, and count > Nelem is evaluated while Nelem is still uninitialized, leading to undefined behavior. This test might pass or fail unpredictably depending on the compiler and stack garbage. Please fix the uninitialized variable bug in src/backend/Storage.cpp as well.
| add_test( | ||
| NAME package_config_exports_linkable_target | ||
| COMMAND ${CMAKE_COMMAND} | ||
| -S ${CMAKE_CURRENT_SOURCE_DIR}/cmake/package_config_consumer | ||
| -B ${CMAKE_CURRENT_BINARY_DIR}/package_config_consumer | ||
| -DCMAKE_PREFIX_PATH=${CMAKE_BINARY_DIR} | ||
| ) |
There was a problem hiding this comment.
The test package_config_exports_linkable_target currently only runs the CMake configuration step (-S ... -B ...). It does not actually build (compile and link) the consumer project.
To verify that the package configuration exposes a truly linkable target (as intended by this test), the project must also be built. We can achieve this in a platform-independent way by adding a second test for the build step and making it depend on the configuration test.
add_test(
NAME package_config_exports_linkable_target_configure
COMMAND ${CMAKE_COMMAND}
-S ${CMAKE_CURRENT_SOURCE_DIR}/cmake/package_config_consumer
-B ${CMAKE_CURRENT_BINARY_DIR}/package_config_consumer
-DCMAKE_PREFIX_PATH=${CMAKE_BINARY_DIR}
)
add_test(
NAME package_config_exports_linkable_target_build
COMMAND ${CMAKE_COMMAND} --build ${CMAKE_CURRENT_BINARY_DIR}/package_config_consumer
)
set_tests_properties(package_config_exports_linkable_target_build
PROPERTIES DEPENDS package_config_exports_linkable_target_configure)
| for (cytnx_uint64 i = 0; i < values_only[0].shape()[0]; ++i) { | ||
| EXPECT_TRUE(abs(values_only[0].at<cytnx_complex128>({i}) - | ||
| with_vectors[0].at<cytnx_complex128>({i})) < 1e-13); | ||
| } |
There was a problem hiding this comment.
Eigenvalues of a non-symmetric matrix returned by linalg::Eig are not guaranteed to be in any specific order, and the order can differ depending on whether eigenvectors are computed or not (or across different BLAS/LAPACK backends).
Comparing them element-by-element directly can lead to flaky test failures in CI/CD environments. It is much more robust to verify that each eigenvalue in values_only has a matching counterpart in with_vectors within the specified tolerance, regardless of their order.
| for (cytnx_uint64 i = 0; i < values_only[0].shape()[0]; ++i) { | |
| EXPECT_TRUE(abs(values_only[0].at<cytnx_complex128>({i}) - | |
| with_vectors[0].at<cytnx_complex128>({i})) < 1e-13); | |
| } | |
| for (cytnx_uint64 i = 0; i < values_only[0].shape()[0]; ++i) { | |
| bool found = false; | |
| auto val = values_only[0].at<cytnx_complex128>({i}); | |
| for (cytnx_uint64 j = 0; j < with_vectors[0].shape()[0]; ++j) { | |
| if (abs(val - with_vectors[0].at<cytnx_complex128>({j})) < 1e-13) { | |
| found = true; | |
| break; | |
| } | |
| } | |
| EXPECT_TRUE(found); | |
| } |
|
I guess your report was generated from abandoned |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new tests for Tensor.Fromfile in Python, package configuration consumer tests in CMake, and unit tests for Tensor_Eig_ValuesOnly in C++. The feedback identifies a critical bug where cy.Tensor.Fromfile is incorrectly bound to cytnx::Tensor::Load in Python, which will cause the new Python test to fail. Additionally, it is recommended to explicitly set the C++17 standard in the package consumer's CMake configuration and to use std::abs instead of abs for complex numbers in the C++ tests to prevent compilation or overload resolution issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| tensor = cy.arange(0, 5, 1, dtype=cy.Type.Double) | ||
|
|
||
| tensor.Tofile(str(path)) | ||
| loaded = cy.Tensor.Fromfile(str(path), cy.Type.Double) |
There was a problem hiding this comment.
The cy.Tensor.Fromfile method in Python is currently bound to cytnx::Tensor::Load in pybind/tensor_py.cpp (line 302) instead of cytnx::Tensor::Fromfile.\n\nBecause Tensor::Load expects a serialized Cytnx tensor format (which starts with the 888 magic header), while Tensor.Tofile writes raw binary data, this test is guaranteed to fail with [ERROR] the object is not a cytnx tensor!.\n\nTo fix this, please update the binding in pybind/tensor_py.cpp to call cytnx::Tensor::Fromfile(fname, dtype, count) instead of cytnx::Tensor::Load(fname).
| @@ -0,0 +1,7 @@ | |||
| cmake_minimum_required(VERSION 3.20) | |||
| project(CytnxPackageConfigConsumer LANGUAGES CXX) | |||
There was a problem hiding this comment.
Since cytnx uses C++17 features (such as std::variant in Tensor.hpp), the consumer project should explicitly require C++17 to ensure successful compilation across all compilers and environments.\n\nPlease set CMAKE_CXX_STANDARD to 17 and CMAKE_CXX_STANDARD_REQUIRED to ON.
project(CytnxPackageConfigConsumer LANGUAGES CXX)\n\nset(CMAKE_CXX_STANDARD 17)\nset(CMAKE_CXX_STANDARD_REQUIRED ON)
| if (!matched[j] && | ||
| abs(value - with_vectors[0].at<cytnx_complex128>({j})) < 1e-13) { |
There was a problem hiding this comment.
Using the bare abs function on a std::complex argument can lead to compilation errors or incorrect overload resolution (e.g., resolving to ::abs(int) from <stdlib.h>) on some compilers. It is safer and more portable to explicitly use std::abs from <complex>.
if (!matched[j] &&\n std::abs(value - with_vectors[0].at<cytnx_complex128>({j})) < 1e-13) {
| #include "cytnx.hpp" | ||
|
|
||
| int main() { | ||
| cytnx::Tensor tensor({1}); | ||
| return tensor.shape()[0] == 1 ? 0 : 1; | ||
| } |
There was a problem hiding this comment.
We already have this test in https://github.com/Cytnx-dev/Cytnx/tree/master/tests/downstream_find_package , and GitHub action has guarded the test.
ianmccul
force-pushed
the
codex/add-regression-tests-issues-880-883
branch
from
67a51dc to
763932b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #884 +/- ##
==========================================
+ Coverage 29.49% 29.54% +0.05%
==========================================
Files 241 241
Lines 35524 35524
Branches 14780 14780
==========================================
+ Hits 10477 10495 +18
+ Misses 17791 17747 -44
- Partials 7256 7282 +26
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
ianmccul
changed the title
2 participants
Fixes two issue reports opened from the repo review. The package-config report in #882 is intentionally excluded because maintainers noted it was already fixed by #809 and #811 and
masteralready hastests/downstream_find_packageplus a dedicated GitHub Action for that path.Fixes #881
Fixes #883
Changes:
Tensor.Fromfile(fname, dtype, count)now dispatches tocytnx::Tensor::Fromfileinstead ofcytnx::Tensor::Load, so rawTensor.Tofileoutput can be read back through the Python API.Eig(..., false)LAPACKE paths initialize the unused eigenvector pointer tonullptrwhen eigenvectors are not requested.linalg::Eig(..., false)eigenvalue comparison.Validation performed locally:
python3 -m py_compile pytests/io/test_tensor_fromfile.pygit diff --checkcmake --build /tmp/cytnx-storage-fromfile-count-build --target test_main -j2/usr/bin/ctest --test-dir /tmp/cytnx-storage-fromfile-count-build -R '^linalg_Test\.Tensor_Eig_ValuesOnly$' --output-on-failurecmake --build /tmp/cytnx-tdvp-python-build --target pycytnx -j2PYTHONPATH=/tmp/cytnx-pydeps:/tmp/cytnx-pr884-testpkg pytest pytests/io/test_tensor_fromfile.py -qfrom a temp test root:1 passed in 0.18sNote: the reused local CMake coverage build emitted gcov checksum warnings while relinking after branch changes; the focused tests passed.