feat: analysis data from csv to evaluate perfomance of 3 ways solve Ax=b#82
Conversation
…lve Ax=b and stability of SPD. Hillbert
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughGauss–Seidel in Changes
Sequence Diagram(s)(omitted — changes are localized to solver implementation and CI exclusion; no multi-component sequential flow requiring diagram) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@part3/solvers.py`:
- Around line 9-10: The gauss_seidel default stopping (tolerance=1e-8,
max_iter=100000) makes the benchmark call gauss_seidel(A, b) use a
size-dependent, overly strict absolute L2 criterion; either change the
gauss_seidel signature defaults to a looser, stable pair (e.g., tolerance=1e-6,
max_iter=10000) so benchmarks are comparable, or modify the benchmark call
gauss_seidel(A, b) to pass explicit tolerance and max_iter values (or switch to
a relative/infinity-norm criterion) so the convergence threshold is controlled
outside gauss_seidel. Ensure the change references the gauss_seidel function
signature and the benchmark invocation gauss_seidel(A, b).
- Around line 16-21: The code checks A_np.shape[1] before confirming A_np is 2-D
and doesn't enforce b_np to be 1-D, causing IndexError or matrix-shaped b; in
gauss_seidel ensure you first validate dimensions: check A_np.ndim == 2 and
raise ValueError if not, then set n = A_np.shape[0] and verify A_np.shape[1] ==
n (square), and also require b_np.ndim == 1 and b_np.shape[0] == n (raise
ValueError otherwise) so later indexing and arithmetic operate on 1-D vectors
and avoid shape index errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d04b1680-b5f7-46d8-9b9d-4f5dd97898cc
⛔ Files ignored due to path filters (1)
part3/results.csvis excluded by!**/*.csv
📒 Files selected for processing (2)
part3/analysis.ipynbpart3/solvers.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
59-77:⚠️ Potential issue | 🟠 Major
--exclude=$PART3_FILESonly excludes one file due to shell word splitting (Line 76).
PART3_FILESexpands to two space-separated paths, so--exclude=$PART3_FILESbecomes two separate arguments:--exclude=part3/solvers.pyandpart3/benchmark.py. The second path becomes a positional argument to grep and gets searched instead of excluded, making the Part 3 bypass incomplete.Suggested fix (exclude Part 3 by construction)
- for f in $PART1_FILES $PART2_FILES $PART3_FILES; do + for f in $PART1_FILES $PART2_FILES; do [ -f "$f" ] && files="$files $f" || echo "Skipping $f (not found)" done if [ -z "$files" ]; then echo "No files found — skipping banned solver check" exit 0 fi ! grep -n \ -e "linalg\.solve" \ -e "linalg\.inv" \ -e "scipy\.linalg\.qr" \ -e "scipy\.linalg\.lu" \ -e "\.rref()" \ -e "\.echelon_form()" \ -e "linsolve(" \ -e ".array" \ - $files \ - --exclude=$PART3_FILES + $files🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 59 - 77, The grep --exclude invocation incorrectly expands PART3_FILES into multiple words so only the first path is treated as an exclude; update the grep call to build one --exclude=... option per path in PART3_FILES (e.g. loop over PART3_FILES and append "--exclude=$p" into an excludes variable) and then call grep with $excludes and $files; reference the variables PART3_FILES, files and the grep command/--exclude flag so the new code constructs multiple --exclude arguments instead of relying on a single expansion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 59-77: The grep --exclude invocation incorrectly expands
PART3_FILES into multiple words so only the first path is treated as an exclude;
update the grep call to build one --exclude=... option per path in PART3_FILES
(e.g. loop over PART3_FILES and append "--exclude=$p" into an excludes variable)
and then call grep with $excludes and $files; reference the variables
PART3_FILES, files and the grep command/--exclude flag so the new code
constructs multiple --exclude arguments instead of relying on a single
expansion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e1ee089-30b1-4777-ae68-c7d7bc2fa3db
📒 Files selected for processing (2)
.github/workflows/ci.ymlpart3/analysis.ipynb
There was a problem hiding this comment.
♻️ Duplicate comments (2)
part3/solvers.py (2)
16-21:⚠️ Potential issue | 🟠 MajorValidate dimensionality before reading
shape[1].
gauss_seidel([], [])andgauss_seidel([1, 2], [3, 4])still fail withIndexErrorhere instead of the intendedValueError, and a column-vectorbwith shape(n, 1)still slips through the current length check. GuardA_np.ndim == 2andb_np.ndim == 1before usingshape[1].Proposed fix
- n = A_np.shape[0] - - if A_np.shape[0] != A_np.shape[1]: + if A_np.ndim != 2: + raise ValueError("A phải là ma trận 2 chiều.") + if b_np.ndim != 1: + raise ValueError("b phải là vector 1 chiều.") + n = A_np.shape[0] + if A_np.shape[1] != n: raise ValueError("A phải là ma trận vuông.") if b_np.shape[0] != n: raise ValueError("Kích thước A và b không khớp.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@part3/solvers.py` around lines 16 - 21, The function gauss_seidel currently accesses A_np.shape[1] and assumes b_np is 1-D without validating dimensions; add explicit guards that A_np.ndim == 2 and b_np.ndim == 1 (or accept b as 1-D vector) before any use of shape[1], and raise ValueError with the existing Vietnamese messages when these checks fail; then validate that A_np.shape[0] == A_np.shape[1] and b_np.shape[0] == n after the ndim checks so calls like gauss_seidel([], []) or column-vector b with shape (n,1) produce the intended ValueError instead of IndexError.
9-10:⚠️ Potential issue | 🟠 MajorKeep benchmark stopping policy out of the solver defaults.
part3/benchmark.py:45-46still callsgauss_seidel(A, b)with no overrides, so these defaults and the absolute||x - x_old||₂/||Ax-b||₂checks directly affect the time-vs-n and error-vs-n plots. That makes larger systems satisfy a stricter per-component threshold, so the benchmark mixes solver behavior with stopping-policy drift. Passtoleranceandmax_iterexplicitly from the benchmark, or switch this check to a relative orord=np.infcriterion.Also applies to: 42-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@part3/solvers.py` around lines 9 - 10, The solver default stopping policy is leaking into benchmarks: update the gauss_seidel signature (the tolerance and max_iter parameters) so the solver does not impose a hidden benchmark-specific default—either remove the hardcoded defaults (or set them to None) and require benchmark.py to call gauss_seidel(A, b, tolerance=..., max_iter=...) with explicit values, or change the stopping test inside gauss_seidel to a relative criterion (e.g., ||x - x_old||₂ / ||x||₂ or use ord=np.inf) so convergence is scale-independent; adjust the benchmark invocation in benchmark.py (where gauss_seidel(A, b) is called) to pass the chosen tolerance and max_iter explicitly.
🧹 Nitpick comments (1)
part3/solvers.py (1)
26-30: Reword this warning for SPD cases.Diagonal dominance is only a sufficient condition here. For the PR's SPD/Hilbert analysis, this warning will flag valid inputs where Gauss–Seidel is still expected to converge, so the message overstates the risk. Consider making it explicitly heuristic or skipping it for known SPD matrices.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@part3/solvers.py` around lines 26 - 30, The existing check on A_np's diagonal dominance should be softened: change the warnings.warn message to a heuristic phrasing (e.g., "Matrix is not strictly diagonally dominant; Gauss–Seidel may still converge for SPD matrices or special cases.") and optionally skip emitting the warning when A_np is detected as symmetric positive definite (check symmetry A_np == A_np.T within tolerance and test SPD via a try/except around np.linalg.cholesky). Update the warning logic around the diag/off_diag_sum check so it only warns for non-SPD matrices or uses the new heuristic message when SPD/Hilbert is detected, and reference the existing variables A_np, diag, off_diag_sum and the Gauss–Seidel context in the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@part3/solvers.py`:
- Around line 16-21: The function gauss_seidel currently accesses A_np.shape[1]
and assumes b_np is 1-D without validating dimensions; add explicit guards that
A_np.ndim == 2 and b_np.ndim == 1 (or accept b as 1-D vector) before any use of
shape[1], and raise ValueError with the existing Vietnamese messages when these
checks fail; then validate that A_np.shape[0] == A_np.shape[1] and b_np.shape[0]
== n after the ndim checks so calls like gauss_seidel([], []) or column-vector b
with shape (n,1) produce the intended ValueError instead of IndexError.
- Around line 9-10: The solver default stopping policy is leaking into
benchmarks: update the gauss_seidel signature (the tolerance and max_iter
parameters) so the solver does not impose a hidden benchmark-specific
default—either remove the hardcoded defaults (or set them to None) and require
benchmark.py to call gauss_seidel(A, b, tolerance=..., max_iter=...) with
explicit values, or change the stopping test inside gauss_seidel to a relative
criterion (e.g., ||x - x_old||₂ / ||x||₂ or use ord=np.inf) so convergence is
scale-independent; adjust the benchmark invocation in benchmark.py (where
gauss_seidel(A, b) is called) to pass the chosen tolerance and max_iter
explicitly.
---
Nitpick comments:
In `@part3/solvers.py`:
- Around line 26-30: The existing check on A_np's diagonal dominance should be
softened: change the warnings.warn message to a heuristic phrasing (e.g.,
"Matrix is not strictly diagonally dominant; Gauss–Seidel may still converge for
SPD matrices or special cases.") and optionally skip emitting the warning when
A_np is detected as symmetric positive definite (check symmetry A_np == A_np.T
within tolerance and test SPD via a try/except around np.linalg.cholesky).
Update the warning logic around the diag/off_diag_sum check so it only warns for
non-SPD matrices or uses the new heuristic message when SPD/Hilbert is detected,
and reference the existing variables A_np, diag, off_diag_sum and the
Gauss–Seidel context in the change.
3 participants
Closes #36, #37
analysis data from csv to evaluate perfomance of 3 ways solve Ax=b and stability of SPD, Hillbert
Summary by CodeRabbit