feat: add comprehensive test suite and verification helper for Part 1 #83
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces a determinant-only loop in Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner
participant Notebook as part1_demo.ipynb
participant Algo as LinearAlg (gaussian_eliminate / inverse / rank_and_basis / determinant)
participant Verifier as verify_system
participant NumPy as NumPy
Runner->>Notebook: execute notebook suites
Notebook->>Algo: call gaussian_eliminate / back_substitute / determinant / inverse / rank_and_basis
Algo-->>Notebook: return results (x, det, A_inv, rank, basis)
Notebook->>Verifier: verify_system(A, x, b, label)
Verifier->>NumPy: attempt np.linalg.solve / compute residuals / np.linalg.det
NumPy-->>Verifier: results or LinAlgError
Verifier-->>Notebook: assert pass or raise
Notebook-->>Runner: print per-suite status and final conclusion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@part1/part1_demo.ipynb`:
- Around line 95-110: The test cells (e.g., test_gaussian_cases which calls
gaussian_eliminate and verify_solution) currently only print results so failures
don't stop the notebook; change them to real assertions by having
verify_solution raise an AssertionError on mismatch or by wrapping its result
with assert (e.g., assert verify_solution(A, x, b)), and replace any prints of
"✅/❌" with assert statements that include a helpful message (e.g., assert
condition, f"Case {name} failed: ..."). Do the same refactor for the other test
blocks referenced (the cells around lines 134-147, 171-186, 210-228, 257-271) so
any failing case raises an exception and halts execution rather than only
printing.
- Around line 212-226: The test block never hits the singular-matrix path of
inverse (function inverse) so add a singular case to the cases list (e.g., a
rank-deficient 2x2 like [[1,2],[2,4]]) and update the exception handling: when
inverse raises ValueError for that case, treat the result as a passing test
(print a PASS message) rather than a bare error; keep the existing np.allclose
check for non-singular cases and only consider ValueError as expected/pass for
the newly added singular case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
spongeofop-rgb
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@part1/part1_demo.ipynb`:
- Around line 10-13: The notebook claims to validate the determinant but never
actually runs assertions; add an assertion-based test suite that calls the
imported determinant function (determinant) and verifies expected outputs for
representative matrices (identity, triangular, singular with det=0, and a small
ill-conditioned example) and compares against known values or numpy.linalg.det
with an appropriate tolerance; place this assertion block near where determinant
is imported and alongside the existing tests for gauss/elimination so the
determinant algorithm is actually verified in the executed flow.
- Around line 114-118: The test suite's test_cases list (variable test_cases in
part1_demo.ipynb) only contains three cases and fails to meet the target of at
least five cases per function path (Gaussian elimination, inverse, rank/basis);
expand each relevant test list (the Gaussian tests around the shown test_cases,
and the similar lists at the other locations referenced) to include at least
five varied matrices per path — e.g., add cases for larger sizes (4x4), singular
variants, near-singular/ill-conditioned, zero/duplicate rows, negative/float
entries, and a random seeded matrix — so each tested function (Gaussian
elimination, matrix inverse, rank/basis functions) has >=5 representative test
inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
part1/part1_demo.ipynb (1)
8-13:⚠️ Potential issue | 🟠 MajorAdd the missing determinant assertion suite.
The notebook still claims Part 1 is verified end-to-end, but
determinant(...)is never exercised anywhere in the executed flow. That leaves one core algorithm unverified despite the intro and import block claiming otherwise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@part1/part1_demo.ipynb` around lines 8 - 13, The notebook never exercises determinant(...), so add an assertion-based test suite that calls determinant for representative cases and integrates with the existing Part 1 assertion runner (e.g., add tests inside the current assert_suite or run_part1_assertions function/block); specifically include checks for identity matrices (det=1), triangular matrices (product of diagonal), singular matrices (det=0), small known examples (explicit integer determinants), and several random and ill-conditioned matrices validated against numpy.linalg.det with a floating-point tolerance (use abs(a-b) < tol). Ensure each assertion references determinant(...) by name and that failures raise informative AssertionErrors so the existing end-to-end verification truly covers determinant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@part1/part1_demo.ipynb`:
- Around line 79-84: The helper currently treats a shape mismatch (len(x) !=
len(A)) as an acceptable success in the except block for np.linalg.LinAlgError;
instead, make the residual the sole deciding condition by removing the shape
check and asserting only that error < 1e-5 (use the existing variables x, A,
error, label in the except block after np.linalg.LinAlgError) so
rectangular/wide systems fail when Ax != b.
---
Duplicate comments:
In `@part1/part1_demo.ipynb`:
- Around line 8-13: The notebook never exercises determinant(...), so add an
assertion-based test suite that calls determinant for representative cases and
integrates with the existing Part 1 assertion runner (e.g., add tests inside the
current assert_suite or run_part1_assertions function/block); specifically
include checks for identity matrices (det=1), triangular matrices (product of
diagonal), singular matrices (det=0), small known examples (explicit integer
determinants), and several random and ill-conditioned matrices validated against
numpy.linalg.det with a floating-point tolerance (use abs(a-b) < tol). Ensure
each assertion references determinant(...) by name and that failures raise
informative AssertionErrors so the existing end-to-end verification truly covers
determinant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
part1/part1_demo.ipynb (1)
70-86:⚠️ Potential issue | 🟠 MajorDocumentation/code mismatch and residual check bypass still present.
Two issues in
verify_system:
Lines 57-60 vs 77: The markdown documentation states the error is computed as relative residual
||Ax-b||/||b||, but line 77 computes absolute residualnp.linalg.norm(residual)without normalization.Line 84: The condition
or len(x) != len(A)still bypasses the residual check for non-square systems. This was flagged in a previous review (marked as addressed) but remains in the code. The "Rectangular Wide" test case (line 125) can pass even ifAx ≠ b.Proposed fix
def verify_system(A, x, b, label="Case"): A_np = np.array(A, dtype=float) x_np = np.array(x, dtype=float) b_np = np.array(b, dtype=float) # Tính sai số Ax - b residual = np.dot(A_np, x_np) - b_np - error = np.linalg.norm(residual) + error = np.linalg.norm(residual) / max(np.linalg.norm(b_np), 1.0) try: x_ref = np.linalg.solve(A_np, b_np) assert np.allclose(x_np, x_ref, atol=1e-5), f"❌ {label}: Nghiệm không khớp với Numpy!" except np.linalg.LinAlgError: # Nếu hệ vô định/vô nghiệm, kiểm tra xem x có thỏa mãn tối ưu không - assert error < 1e-5 or len(x) != len(A), f"❌ {label}: Sai số quá lớn!" + assert error < 1e-5, f"❌ {label}: Sai số quá lớn!" print(f" [OK] {label}: Verified (Error: {error:.2e})")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@part1/part1_demo.ipynb` around lines 70 - 86, The verify_system function currently computes an absolute residual and bypasses checks for non-square systems; change it to compute a relative residual by setting error = np.linalg.norm(residual) / (np.linalg.norm(b_np) + eps) (use a small eps to avoid division by zero) instead of error = np.linalg.norm(residual), and remove the len(x) != len(A) bypass; for rectangular or singular cases, compute a reference solution x_ref via np.linalg.lstsq(A_np, b_np, rcond=None)[0] (fall back to np.linalg.solve when appropriate) and compare x_np to x_ref with np.allclose using the same atol, and keep the LinAlgError handling only for genuine solve failures while using the relative error for assertions and reporting.
🧹 Nitpick comments (1)
part1/part1_demo.ipynb (1)
152-155: Redundant imports.
mathandnumpyare already imported at lines 34-35. These re-imports are unnecessary.Proposed fix
- "import math\n", - "import numpy as np\n", - "\n", "print(\"--- Testing Determinant Suite ---\")\n",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@part1/part1_demo.ipynb` around lines 152 - 155, Remove the redundant re-imports of math and numpy by deleting the repeated lines "import math" and "import numpy as np" found in the cell that repeats those imports; keep the original imports at their first occurrence (the earlier cell where math and numpy are imported) so any code using math or np continues to work, and run the notebook to confirm no NameError occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@part1/part1_demo.ipynb`:
- Around line 70-86: The verify_system function currently computes an absolute
residual and bypasses checks for non-square systems; change it to compute a
relative residual by setting error = np.linalg.norm(residual) /
(np.linalg.norm(b_np) + eps) (use a small eps to avoid division by zero) instead
of error = np.linalg.norm(residual), and remove the len(x) != len(A) bypass; for
rectangular or singular cases, compute a reference solution x_ref via
np.linalg.lstsq(A_np, b_np, rcond=None)[0] (fall back to np.linalg.solve when
appropriate) and compare x_np to x_ref with np.allclose using the same atol, and
keep the LinAlgError handling only for genuine solve failures while using the
relative error for assertions and reporting.
---
Nitpick comments:
In `@part1/part1_demo.ipynb`:
- Around line 152-155: Remove the redundant re-imports of math and numpy by
deleting the repeated lines "import math" and "import numpy as np" found in the
cell that repeats those imports; keep the original imports at their first
occurrence (the earlier cell where math and numpy are imported) so any code
using math or np continues to work, and run the notebook to confirm no NameError
occurs.
TanKhoiTV
changed the title
aFlyingSeal
approved these changes
Apr 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue
Closes #20
This PR completes the comprehensive walkthrough for Part 1 and verify
Checklist
Code
numpy.linalg,scipy.linalg, orsympysolver calls in implementation codepathlib.Path, no hardcoded OS separatorsprint()statements left inTests
Summary by CodeRabbit