Fix matrix index bounds validation - #140
Conversation
Co-authored-by: ryanxw <16125496+ryanxw@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens matrix index bounds validation across the C++ TinyMatrix API and the Python bindings for chi matrices, and adds regression tests to ensure out-of-range access raises the expected exceptions.
Changes:
iqs::TinyMatrix::operator()now throwsstd::out_of_rangeinstead of relying onassertfor bounds enforcement.- Pybind11
__getitem__/__setitem__forCM4x4andCM16x16now correctly validate indices (including negatives) and raiseIndexError. - Adds C++ and Python tests for out-of-range matrix access; updates Dockerfile to use keyrings instead of
apt-key.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| include/tinymatrix.hpp | Switches TinyMatrix bounds checks from asserts to std::out_of_range throws. |
| unit_test/include/tinymatrix_test.hpp | Adds a unit test verifying out-of-range access throws for const/non-const matrices. |
| pybind11/intelqs_py.cpp | Fixes Python index validation for chi matrices and removes an unusable __getitem__ signature. |
| unit_test/import_iqs.py | Adds Python-level checks ensuring invalid indices raise IndexError. |
| Dockerfile | Replaces deprecated apt-key usage with a signed-by keyring setup for MKL repo. |
Suppressed comments (1)
include/tinymatrix.hpp:157
- The non-const operator()(i,j) doc still lists only a precondition, but the function now throws std::out_of_range on invalid indices. Updating the comment helps keep the public API contract accurate.
/// Access a matrix element.
/// \param i the row index
/// \param j the column index
/// \pre i<numRows() & j<numCols()
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -141,8 +142,10 @@ class TinyMatrix | |||
| /// \pre i<numRows() & j<numCols() | |||
There was a problem hiding this comment.
Updated both accessor contracts to document std::out_of_range in commit aed9dc1.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: ryanxw <16125496+ryanxw@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Dockerfile:44
apt-get install -y gpgis run without anapt-get updatein the same layer. If earlierapt-get updatelayers are cached, package indexes can become stale and the build can fail when installinggpg(404s). Runapt-get updateimmediately before installinggpg(ideally in the same RUN instruction).
RUN apt-get install -y gpg
|
It seems that the docker image contains an old version of cmake (3.15.2) while 3.16 is required. |
giangiac
left a comment
There was a problem hiding this comment.
All changes are meaningful.
No description provided.