Port SIES to Python#2
Conversation
- Implemented core shape system with DSL support for translation, scaling, and rotation. - Implemented acquisition configurations and Laplacian tools. - Implemented Conductivity_R2 PDE solver and CGPT reconstruction. - Added comprehensive Numpydoc docstrings. - Created a Marimo notebook demo replicating the original Matlab example. - Added unit tests and setup pyproject.toml. Co-authored-by: tomboulier <14161960+tomboulier@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
WalkthroughCette pull request introduit une nouvelle librairie SIES pour simuler et reconstruire des propriétés conductrices via des acquisitions électriques. Elle ajoute ~1400 lignes de code couvrant des géométries d'inclusions, des configurations d'acquisition, des solveurs PDE et une décomposition asymptotique, avec un exemple intégré et des tests de validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Demo as Script Demo
participant Cond as Conductivity_R2
participant Asymp as Asymp Module
participant Solver as LSQR Solver
Demo->>Cond: Initialiser avec Ellipse, cnd, pmtt
Cond->>Cond: Construire KsdS (matrices noyau bloc)
Demo->>Cond: data_simulation(freq_list)
loop Pour chaque fréquence
Cond->>Asymp: lambda_contrast(cnd, freq)
Asymp-->>Cond: λ(freq)
Cond->>Cond: _compute_phi(freq)
Cond->>Cond: Évaluer MSR
end
Cond-->>Demo: MSR_list
Demo->>Demo: add_white_noise(MSR_list)
Demo-->>Demo: MSR_noisy
Demo->>Asymp: theoretical_CGPT(inclusions, λ, ord)
Asymp->>Asymp: Assembler système Amat
Asymp->>Asymp: Résoudre pour chaque mode m
Asymp-->>Demo: CGPT_theory
Demo->>Cond: reconstruct_CGPT(MSR_noisy, ord, method='lsqr')
Cond->>Cond: make_linop_CGPT(ord)
Cond->>Solver: Appeler LSQR(A, b, maxiter, tol)
Solver-->>Cond: x_reconstructed
Cond-->>Demo: CGPT_recon, résidus
Demo->>Demo: Comparer || CGPT_theory - CGPT_recon ||
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Observations bienveillantes d'un tech lead : Ah, belle initiative ! 🚀 Quelques bonnes pratiques à consolider ici :
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR ports the core of the SIES (Shape Identification in Electro-Sensing) library from MATLAB to Python, providing a sies package with shape modeling, acquisition configuration, PDE solvers, and asymptotic expansion tools for conductivity problems.
Changes:
- Implements the full Python package structure (
sies/) with submodules for shapes (Shape,Ellipse,Triangle,Flower), acquisition systems (Concentric,Coincided), PDE solvers (Conductivity_R2), asymptotic tools (theoretical_CGPT,lambda_contrast), and utility functions (Green2D, noise addition). - Adds a Marimo notebook demo (
examples/conductivity_demo.py) that replicates the MATLAB conductivity simulation and CGPT reconstruction workflow. - Adds a test suite (
tests/test_sies.py) with three tests covering shape DSL operations, conductivity data simulation, and CGPT reconstruction.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
sies/__init__.py |
Top-level package exports for Shape, Acquisition, PDE, and Asymptotic modules |
sies/shape/base.py |
Base Shape class with boundary representation, DSL operators (+, *, @), and interpolation utilities |
sies/shape/ellipse.py |
Ellipse shape subclass |
sies/shape/triangle.py |
Triangle shape subclass with corner smoothing |
sies/shape/flower.py |
Flower shape subclass with petal perturbation |
sies/shape/__init__.py |
Shape subpackage exports |
sies/acq/__init__.py |
Acquisition system classes (AcquisitionConfig, Concentric, Coincided) |
sies/asymp/__init__.py |
Asymptotic expansion tools: lambda contrast, kernel matrices, CGPT computation |
sies/pde/base.py |
Base PDE solver class (SmallInclusions), Green's function, single-layer potential |
sies/pde/conductivity.py |
Conductivity_R2 solver for conductivity problems with forward simulation and CGPT reconstruction |
sies/pde/__init__.py |
PDE subpackage exports |
sies/tools/__init__.py |
Utility functions for adding white noise |
sies/tools/laplacian/__init__.py |
2D Green's function gradient and normal derivative |
tests/test_sies.py |
Test suite for shape DSL, data simulation, and CGPT reconstruction |
examples/conductivity_demo.py |
Marimo notebook demo for conductivity simulation workflow |
pyproject.toml |
Package configuration with dependencies and build system |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CGPT = X.reshape((2 * ord_val, 2 * ord_val), order='F') | ||
|
|
||
| if symmode: | ||
| CGPT = (CGPT + CGPT.T) | ||
|
|
||
| out["CGPT"].append(CGPT) | ||
| residual = msr - op.L(CGPT, 'notransp').reshape((Ns, Nr), order='F') | ||
| out["res"].append(np.linalg.norm(residual, 'fro')) | ||
| out["rres"].append(out["res"][-1] / np.linalg.norm(msr, 'fro')) |
There was a problem hiding this comment.
Bug: When symmode=True, the residual computation double-symmetrizes the CGPT matrix. The op.L function (created with symmode=True) internally computes As @ (Xm + Xm.T) @ Ar.T. After line 234, CGPT is already symmetrized (CGPT_raw + CGPT_raw.T), so passing it to op.L computes As @ (CGPT + CGPT.T) @ Ar.T = As @ 2*CGPT @ Ar.T, which is incorrect.
To match the MATLAB behavior, the residual should be computed before the symmetrization step (i.e., move lines 237-239 before the if symmode: block on line 233), using the raw LSQR solution.
| CGPT = X.reshape((2 * ord_val, 2 * ord_val), order='F') | |
| if symmode: | |
| CGPT = (CGPT + CGPT.T) | |
| out["CGPT"].append(CGPT) | |
| residual = msr - op.L(CGPT, 'notransp').reshape((Ns, Nr), order='F') | |
| out["res"].append(np.linalg.norm(residual, 'fro')) | |
| out["rres"].append(out["res"][-1] / np.linalg.norm(msr, 'fro')) | |
| CGPT_raw = X.reshape((2 * ord_val, 2 * ord_val), order='F') | |
| # Compute residual using raw (unsymmetrized) LSQR solution to avoid double symmetrization | |
| residual = msr - op.L(CGPT_raw, 'notransp').reshape((Ns, Nr), order='F') | |
| out["res"].append(np.linalg.norm(residual, 'fro')) | |
| out["rres"].append(out["res"][-1] / np.linalg.norm(msr, 'fro')) | |
| CGPT = CGPT_raw | |
| if symmode: | |
| CGPT = (CGPT + CGPT.T) | |
| out["CGPT"].append(CGPT) |
| CC[m-1, n-1] += np.sum(zn.real * realphim[:, i].real) | ||
| CS[m-1, n-1] += np.sum(zn.imag * realphim[:, i].real) | ||
| SC[m-1, n-1] += np.sum(zn.real * imagphim[:, i].real) | ||
| SS[m-1, n-1] += np.sum(zn.imag * imagphim[:, i].real) |
There was a problem hiding this comment.
Bug: When lambda_val is complex (i.e., freq > 0 and pmtt > 0), the system matrix Amat is complex, so realphim and imagphim will also be complex. Taking .real of these arrays (e.g., realphim[:, i].real) discards the imaginary part, which differs from the MATLAB code where real(zn)*realphim(:,i) performs a full complex dot product. Additionally, the CC/CS/SC/SS arrays are initialized as real (np.zeros(...)) which would fail or silently truncate when accumulating complex values.
To match the MATLAB behavior, remove the .real from realphim[:, i].real and imagphim[:, i].real on all four lines, and initialize CC, CS, SC, SS with dtype=complex. The cell2mat function's M array should also be initialized as complex.
| dependencies = [ | ||
| "numpy", | ||
| "scipy", | ||
| "matplotlib", |
There was a problem hiding this comment.
marimo is listed as a core dependency, but it is only used in the example notebook (examples/conductivity_demo.py), not by the library itself. Consider moving it to an optional dependency group (e.g., [project.optional-dependencies] with a examples extra) so that users who only need the library don't have to install it.
| "matplotlib", | |
| "matplotlib", | |
| ] | |
| [project.optional-dependencies] | |
| examples = [ |
| idx = np.arange(0, D0.shape[1], dspl) | ||
|
|
||
| theta = np.linspace(theta0[0], theta0[-1], nb_points, endpoint=False) |
There was a problem hiding this comment.
Bug: The rescale method has two discrepancies with the MATLAB implementation:
-
Down-sampling indices: The MATLAB code is
idx = [1:dspl:(nbPoints0-1), nbPoints0], which always includes the last point to ensure the periodic boundary spline has correct coverage. The Python codenp.arange(0, D0.shape[1], dspl)may omit the last point. -
Theta resampling: The MATLAB code uses
linspace(theta0(1), theta0(end), nbPoints)which includes the endpoint (default MATLAB behavior). The Python code usesendpoint=False, which produces a different sampling grid. The MATLAB version recovers the original theta0 grid whennb_points == len(theta0), but the Python version does not.
The idx should be something like np.unique(np.append(np.arange(0, D0.shape[1]-1, dspl), D0.shape[1]-1)), and the linspace should use the default endpoint=True (or remove endpoint=False) to match the MATLAB semantics.
| idx = np.arange(0, D0.shape[1], dspl) | |
| theta = np.linspace(theta0[0], theta0[-1], nb_points, endpoint=False) | |
| idx = np.unique( | |
| np.append( | |
| np.arange(0, D0.shape[1] - 1, dspl), | |
| D0.shape[1] - 1 | |
| ) | |
| ) | |
| theta = np.linspace(theta0[0], theta0[-1], nb_points) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
pyproject.toml (1)
7-12: Pense à épingler les versions de tes dépendances pour la reproductibilité.En tant que tech lead, je te conseille vivement d'épingler les versions de tes dépendances (par exemple
numpy>=1.24,<2.0). Cela évite les mauvaises surprises lors des mises à jour automatiques et garantit que ton code fonctionne de manière identique sur toutes les machines.De plus,
marimoest utilisé uniquement pour la démo interactive. Tu pourrais le déplacer vers des dépendances optionnelles :♻️ Proposition de refactoring
dependencies = [ - "numpy", - "scipy", - "matplotlib", - "marimo", + "numpy>=1.24", + "scipy>=1.10", + "matplotlib>=3.7", ] + +[project.optional-dependencies] +dev = ["marimo>=0.1"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 7 - 12, Épingler les versions des paquets listés dans la section dependencies du pyproject (remplacer "numpy", "scipy", "matplotlib" par des contraintes de version stables comme "numpy>=1.24,<2.0" etc.) et déplacer "marimo" des dépendances principales vers des dépendances optionnelles/extras (p.ex. under project.optional-dependencies or extras) afin qu'il soit installé seulement pour la démo; modifie les entrées correspondantes dans pyproject.toml (références: la clé dependencies et le nom du paquet "marimo") pour assurer reproductibilité et séparation des dépendances de démo.examples/conductivity_demo.py (1)
67-91: Bonne démonstration, mais extrais les "magic numbers" pour plus de clarté.Ta démo est bien structurée ! Cependant, en tant que bonne pratique pédagogique (surtout pour un exemple), je te suggère d'extraire les paramètres de reconstruction en constantes nommées au début de la cellule :
# Reconstruction parameters NOISE_LEVEL = 0.01 LSQR_MAX_ITER = 100_000 LSQR_TOL = 1e-10Cela rend le code auto-documenté et facilite l'expérimentation pour les utilisateurs qui voudront modifier ces valeurs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/conductivity_demo.py` around lines 67 - 91, In the app.cell function __ replace the inline magic numbers (nlvl = 0.01, maxiter=100000, tol=1e-10) with named constants defined at the top of the cell (e.g., NOISE_LEVEL, LSQR_MAX_ITER, LSQR_TOL) and use those constants when creating data_noisy (P.add_white_noise(..., NOISE_LEVEL)) and when calling P.reconstruct_CGPT(..., maxiter=LSQR_MAX_ITER, tol=LSQR_TOL); this keeps the reconstruction parameters self-documenting and easy to tweak for users.sies/tools/laplacian/__init__.py (1)
18-24: Bonne gestion de la division par zéro, mais documente ce comportement.Tu as bien géré le cas où
X == Yen assignantnp.infau dénominateur, ce qui produit un gradient nul pour les points coïncidents. C'est une approche pragmatique !Cependant, en tant que bonne pratique, je te suggère d'ajouter un commentaire expliquant le comportement attendu (gradient = 0 pour points coïncidents) afin que les futurs développeurs comprennent immédiatement ce choix.
DN = XY1**2 + XY2**2 - # Handle zero division if any (e.g. X == Y) + # Handle zero division if any (e.g. X == Y): returns Gx=Gy=0 for coincident points DN[DN == 0] = np.inf🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sies/tools/laplacian/__init__.py` around lines 18 - 24, Add a short inline comment near the DN zero-handling (where DN[DN == 0] = np.inf) documenting that setting the denominator to np.inf ensures Gx and Gy (computed from XY1/DN and XY2/DN) evaluate to 0 for coincident points (X == Y), and state this is the intended behavior to avoid division-by-zero and produce a zero gradient at those locations.sies/pde/base.py (1)
2-2: Import inutilisé :Green2D_Grad.
Green2D_Gradest importé mais n'est pas utilisé dans ce fichier. Soit tu as prévu de l'utiliser prochainement, soit tu peux le supprimer pour garder le code propre.-from ..tools.laplacian import Green2D_Dn, Green2D_Grad +from ..tools.laplacian import Green2D_Dn🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sies/pde/base.py` at line 2, Supprime l'import inutilisé Green2D_Grad depuis la ligne "from ..tools.laplacian import Green2D_Dn, Green2D_Grad" dans sies/pde/base.py (ou, si tu prévois de l'utiliser, ajoute l'appel/usage correspondant) : soit retire Green2D_Grad de la liste d'import pour ne conserver que Green2D_Dn, soit implémente son utilisation là où Green2D_Grad est nécessaire.sies/tools/__init__.py (1)
18-30: Ajoute un paramètre pour la reproductibilité des résultats.En tant que tech lead, je te rappelle que
np.random.randnutilise l'état global du générateur aléatoire, ce qui rend les tests non reproductibles. Une bonne pratique est de permettre l'injection d'un générateur ou d'une seed :♻️ Proposition avec générateur optionnel
-def add_white_noise_mat(X, nlvl, mode): +def add_white_noise_mat(X, nlvl, mode, rng=None): + if rng is None: + rng = np.random.default_rng() M, N = X.shape if mode: Y = np.zeros_like(X) for c in range(N): t0 = np.linalg.norm(X[:, c]) / np.sqrt(M) - Y[:, c] = X[:, c] + np.random.randn(M) * t0 * nlvl + Y[:, c] = X[:, c] + rng.standard_normal(M) * t0 * nlvl else: t0 = np.linalg.norm(X, 'fro') / np.sqrt(M * N) - Y = X + np.random.randn(M, N) * t0 * nlvl + Y = X + rng.standard_normal((M, N)) * t0 * nlvlCela permet aux tests d'utiliser
rng=np.random.default_rng(42)pour des résultats déterministes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sies/tools/__init__.py` around lines 18 - 30, The function add_white_noise_mat currently uses np.random.randn which relies on global RNG; add an optional parameter (e.g., rng=None or seed=None) to allow deterministic draws and maintain backward compatibility: if rng is None create a local rng via np.random.default_rng(seed) or default_rng() and then replace np.random.randn calls with rng.standard_normal or rng.normal with the appropriate shape; update return behavior to be unchanged (Y, sigma) and ensure both mode True and False branches use the injected rng (refer to add_white_noise_mat, variable names X, M, N, t0, nlvl).sies/shape/flower.py (1)
38-46: Formules mathématiques correctes, mais difficiles à lire.Les expressions pour la vitesse et l'accélération sont longues. Pour améliorer la lisibilité et faciliter la maintenance, tu pourrais extraire des variables intermédiaires :
♻️ Suggestion de refactoring pour la lisibilité
# Intermediate terms for readability sin_t, cos_t = np.sin(theta), np.cos(theta) sin_nt, cos_nt = np.sin(nb_petals * theta), np.cos(nb_petals * theta) cos_nt_km1 = cos_nt ** (k - 1) # k=1 → cos_nt^0 = 1 # Velocity (tangent) radial_deriv = k * epsilon * nb_petals * sin_nt * cos_nt_km1 tx = -a * (sin_t * radial_pert + radial_deriv * cos_t) ty = b * (cos_t * radial_pert - radial_deriv * sin_t)C'est optionnel, mais cela rend le code plus proche de la notation mathématique.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sies/shape/flower.py` around lines 38 - 46, The velocity and acceleration expressions (tx, ty, tvec, ax, ay, avec) are correct but hard to read; refactor by extracting intermediate terms (e.g., sin_t = np.sin(theta), cos_t = np.cos(theta), sin_nt = np.sin(nb_petals*theta), cos_nt = np.cos(nb_petals*theta), cos_nt_km1 = cos_nt**(k-1)) and compute radial_deriv = k * epsilon * nb_petals * sin_nt * cos_nt_km1, then rewrite tx and ty using sin_t, cos_t, radial_pert and radial_deriv, and rewrite ax and ay using the same intermediate sin/cos terms and nb_petals**2 factors so the long formulas are clearer and easier to maintain.sies/shape/ellipse.py (1)
11-12: Validation correcte, mais envisage une exception personnalisée pour ton domaine.Ta validation
a >= best appropriée pour respecter la convention semi-major/semi-minor. L'avertissement TRY003 du linter suggère d'éviter les messages longs dans les exceptions standards.Pour une librairie scientifique, tu pourrais créer une exception dédiée
ShapeValidationErrordans ton modulesies.shape. C'est optionnel mais améliore la maintenabilité si tu prévois d'ajouter d'autres validations :# Dans sies/shape/base.py ou exceptions.py class ShapeValidationError(ValueError): """Raised when shape parameters are invalid.""" pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sies/shape/ellipse.py` around lines 11 - 12, Replace the generic ValueError raised in the ellipse parameter check with a domain-specific exception: define a ShapeValidationError (subclassing ValueError) in your shape module (e.g., in sies.shape.base or sies.shape.exceptions) and then import and raise ShapeValidationError from the ellipse validation (the code that currently does `if a < b: raise ValueError("Semi-major axis a must be >= semi-minor axis b")`), keeping the same short message but using the new exception class to centralize shape validation errors.tests/test_sies.py (1)
31-50: Ajoute un caslsqret un cas fréquentiel complexe.Ce test ne couvre aujourd’hui que
method='pinv'avecfreq=0etpmtt=0, donc ni le chemin par défautlsqrni les CGPT complexes ne sont verrouillés. Un petit test paramétré éviterait que ces régressions passent silencieusement.Exemple de paramétrisation
- def test_cgpt_reconstruction(): + `@pytest.mark.parametrize`( + ("method", "pmtt", "freq"), + [("pinv", 0.0, 0), ("lsqr", 0.2, 10)], + ) + def test_cgpt_reconstruction(method, pmtt, freq): # Use a simple circle for very stable reconstruction D = [Ellipse(0.5, 0.5, 256)] cfg = Coincided([0, 0], 10, 64) - P = Conductivity_R2(D, [5.0], [0.0], cfg) + P = Conductivity_R2(D, [5.0], [pmtt], cfg) - freq = 0 data = P.data_simulation([freq]) @@ - out = P.reconstruct_CGPT(data["MSR"], ord_val, method='pinv') + out = P.reconstruct_CGPT(data["MSR"], ord_val, method=method)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_sies.py` around lines 31 - 50, Update the test_cgpt_reconstruction to parametrize and cover both the default 'lsqr' path and complex-frequency CGPTs: run the same reconstruction twice (or parameterize) using P.reconstruct_CGPT with method='pinv' and method='lsqr' and also include a nonzero freq (e.g., freq=1) where lambda_contrast will produce complex values so theoretical_CGPT and data_simulation produce complex CGPTs; verify M_rec matches M_theo for both methods using the same relative-norm assertion (use Conductivity_R2, Coincided, lambda_contrast, theoretical_CGPT and reconstruct_CGPT names to locate code). Ensure you adjust any pmtt or frequency inputs passed to Conductivity_R2/data_simulation so the complex path is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sies/asymp/__init__.py`:
- Around line 117-156: The accumulation discards imaginary parts by taking .real
of realphim/imagphim and thus forces CC/CS/SC/SS to be purely real when
lambda_contrast() is complex; fix by keeping complex phi fields and using the
full complex arrays when computing zn * phi (i.e., do not apply .real to
realphim or imagphim, and compute contributions using complex multiplication and
then take real/imag parts only when assigning to CC/CS/SC/SS as needed). Update
the solve/results flow (phi_m_real_vec, phi_m_imag_vec -> combine into complex
phi_m_vec = phi_m_real_vec + 1j*phi_m_imag_vec, reshape into complex
realphim/imagphim or single complex phim) and use that complex phim in the
CC/CS/SC/SS accumulation; ensure any zero-padding logic for b_real/b_imag
preserves complex shape.
In `@sies/pde/base.py`:
- Around line 43-54: Green2D currently calls np.log(DN) and will produce -inf
for coincident points (DN==0); modify Green2D to guard against DN==0 before
taking the log (e.g. replace zeros with a small positive value or use np.where
to set DN_safe = np.where(DN==0, eps, DN)) so np.log never receives 0; refer to
the local symbols DN and G in function Green2D and use a stable epsilon like
np.finfo(float).eps (or mirror the same handling used in Green2D_Grad) so the
function returns finite values without warnings.
In `@sies/pde/conductivity.py`:
- Around line 240-245: La branche method=='pinv' calcule CGPT sans tenir compte
de l'option symmode; corrigez-la pour appliquer la même contrainte symétrique
que l'autre méthode: calculez d'abord CGPT_raw = np.linalg.pinv(op.As) @ msr @
np.linalg.pinv(op.Ar.T), puis si symmode est activé projetez CGPT_raw sur
l'espace symétrique (par ex. via symmetrisation/averaging ou en appliquant la
même fonction de projection utilisée ailleurs) et affectez le résultat à CGPT
avant d'ajouter out["CGPT"], out["res"] et out["rres"]; modifiez la branche où
apparaissent op.As, op.Ar, CGPT et la clé method=='pinv' pour réutiliser la
logique de projection symmode existante afin d'unifier la sémantique des deux
méthodes.
- Around line 233-239: Le calcul du résiduel double la prédiction quand symmode
est True car vous symétrisez CGPT (CGPT = CGPT + CGPT.T) puis appelez op.L(...,
'notransp') qui réapplique X+X.T; corrigez en conservant la version non
symétrisée pour la prédiction : avant la ligne qui fait CGPT = (CGPT + CGPT.T)
sauvegardez CGPT_raw = CGPT, puis utilisez op.L(CGPT_raw, 'notransp') pour
calculer residual et les sorties out["res"] / out["rres"] (ou alternativement
appelez op.L(CGPT/2, 'notransp') si vous préférez ne pas stocker CGPT_raw);
référez-vous aux symboles symmode, CGPT, CGPT_raw (nouveau), op.L, msr,
out["res"] et out["rres"] pour localiser et appliquer la correction.
- Around line 149-155: The assignment is dividing a Python list by a scalar
(causing TypeError); change the RHS to a NumPy array so elementwise division
works and the shape matches the slice A[n, 2*(m-1):2*m]; specifically replace
the list [np.cos(m*T), np.sin(m*T)] / (2 * np.pi * m * R**m) with a NumPy array
expression like np.array([np.cos(m*T), np.sin(m*T)]) / (2 * np.pi * m * R**m)
inside the loop (the issue occurs where A is assigned in the loop that
constructs A, e.g., in the function that contains the for n in range(N) / for m
in range(1, order+1) block).
In `@sies/shape/base.py`:
- Around line 253-255: La spline périodique est fermée sur une pseudo-période
mais rescale() ne demande les échantillons que jusqu’à theta0[-1], ce qui laisse
hors échantillonnage le dernier segment si dspl ne divise pas exactement la
période ; corrigez en faisant en sorte que la variable périodique utilisée pour
l’interpolation (ici t0_periodic) couvre la période complète (inclure
explicitement theta0[-1] + step_used) et assurez-vous que rescale()/le code qui
calcule les nouveaux theta demande des valeurs jusqu’à ce t0_periodic[-1] (et
non theta0[-1]) ; appliquez la même modification aux blocs similaires référencés
(les usages autour de p0_periodic/t0_periodic aux lignes équivalentes 311-313)
et garantissez que l’échantillonnage final prend en compte ce dernier point
périodique.
In `@sies/shape/triangle.py`:
- Around line 66-70: La méthode Triangle.__mul__ utilise super().__mul__() qui
construit un Shape puis réattache lside/angl, ce qui perd le type Triangle;
changez __mul__ de façon à instancier et renvoyer un vrai Triangle (copier
lside, angl et tout autre attribut spécifique comme dspl) au lieu d'utiliser
super().__mul__(), et si dspl doit être conservé assurez-vous qu'il est
enregistré sur self dans __init__ puis copié dans le nouvel objet créé par
__mul__.
---
Nitpick comments:
In `@examples/conductivity_demo.py`:
- Around line 67-91: In the app.cell function __ replace the inline magic
numbers (nlvl = 0.01, maxiter=100000, tol=1e-10) with named constants defined at
the top of the cell (e.g., NOISE_LEVEL, LSQR_MAX_ITER, LSQR_TOL) and use those
constants when creating data_noisy (P.add_white_noise(..., NOISE_LEVEL)) and
when calling P.reconstruct_CGPT(..., maxiter=LSQR_MAX_ITER, tol=LSQR_TOL); this
keeps the reconstruction parameters self-documenting and easy to tweak for
users.
In `@pyproject.toml`:
- Around line 7-12: Épingler les versions des paquets listés dans la section
dependencies du pyproject (remplacer "numpy", "scipy", "matplotlib" par des
contraintes de version stables comme "numpy>=1.24,<2.0" etc.) et déplacer
"marimo" des dépendances principales vers des dépendances optionnelles/extras
(p.ex. under project.optional-dependencies or extras) afin qu'il soit installé
seulement pour la démo; modifie les entrées correspondantes dans pyproject.toml
(références: la clé dependencies et le nom du paquet "marimo") pour assurer
reproductibilité et séparation des dépendances de démo.
In `@sies/pde/base.py`:
- Line 2: Supprime l'import inutilisé Green2D_Grad depuis la ligne "from
..tools.laplacian import Green2D_Dn, Green2D_Grad" dans sies/pde/base.py (ou, si
tu prévois de l'utiliser, ajoute l'appel/usage correspondant) : soit retire
Green2D_Grad de la liste d'import pour ne conserver que Green2D_Dn, soit
implémente son utilisation là où Green2D_Grad est nécessaire.
In `@sies/shape/ellipse.py`:
- Around line 11-12: Replace the generic ValueError raised in the ellipse
parameter check with a domain-specific exception: define a ShapeValidationError
(subclassing ValueError) in your shape module (e.g., in sies.shape.base or
sies.shape.exceptions) and then import and raise ShapeValidationError from the
ellipse validation (the code that currently does `if a < b: raise
ValueError("Semi-major axis a must be >= semi-minor axis b")`), keeping the same
short message but using the new exception class to centralize shape validation
errors.
In `@sies/shape/flower.py`:
- Around line 38-46: The velocity and acceleration expressions (tx, ty, tvec,
ax, ay, avec) are correct but hard to read; refactor by extracting intermediate
terms (e.g., sin_t = np.sin(theta), cos_t = np.cos(theta), sin_nt =
np.sin(nb_petals*theta), cos_nt = np.cos(nb_petals*theta), cos_nt_km1 =
cos_nt**(k-1)) and compute radial_deriv = k * epsilon * nb_petals * sin_nt *
cos_nt_km1, then rewrite tx and ty using sin_t, cos_t, radial_pert and
radial_deriv, and rewrite ax and ay using the same intermediate sin/cos terms
and nb_petals**2 factors so the long formulas are clearer and easier to
maintain.
In `@sies/tools/__init__.py`:
- Around line 18-30: The function add_white_noise_mat currently uses
np.random.randn which relies on global RNG; add an optional parameter (e.g.,
rng=None or seed=None) to allow deterministic draws and maintain backward
compatibility: if rng is None create a local rng via np.random.default_rng(seed)
or default_rng() and then replace np.random.randn calls with rng.standard_normal
or rng.normal with the appropriate shape; update return behavior to be unchanged
(Y, sigma) and ensure both mode True and False branches use the injected rng
(refer to add_white_noise_mat, variable names X, M, N, t0, nlvl).
In `@sies/tools/laplacian/__init__.py`:
- Around line 18-24: Add a short inline comment near the DN zero-handling (where
DN[DN == 0] = np.inf) documenting that setting the denominator to np.inf ensures
Gx and Gy (computed from XY1/DN and XY2/DN) evaluate to 0 for coincident points
(X == Y), and state this is the intended behavior to avoid division-by-zero and
produce a zero gradient at those locations.
In `@tests/test_sies.py`:
- Around line 31-50: Update the test_cgpt_reconstruction to parametrize and
cover both the default 'lsqr' path and complex-frequency CGPTs: run the same
reconstruction twice (or parameterize) using P.reconstruct_CGPT with
method='pinv' and method='lsqr' and also include a nonzero freq (e.g., freq=1)
where lambda_contrast will produce complex values so theoretical_CGPT and
data_simulation produce complex CGPTs; verify M_rec matches M_theo for both
methods using the same relative-norm assertion (use Conductivity_R2, Coincided,
lambda_contrast, theoretical_CGPT and reconstruct_CGPT names to locate code).
Ensure you adjust any pmtt or frequency inputs passed to
Conductivity_R2/data_simulation so the complex path is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80112ad5-730b-417d-84ea-4866d7055e78
📒 Files selected for processing (16)
examples/conductivity_demo.pypyproject.tomlsies/__init__.pysies/acq/__init__.pysies/asymp/__init__.pysies/pde/__init__.pysies/pde/base.pysies/pde/conductivity.pysies/shape/__init__.pysies/shape/base.pysies/shape/ellipse.pysies/shape/flower.pysies/shape/triangle.pysies/tools/__init__.pysies/tools/laplacian/__init__.pytests/test_sies.py
| CC = np.zeros((ord_val, ord_val)) | ||
| CS = np.zeros((ord_val, ord_val)) | ||
| SC = np.zeros((ord_val, ord_val)) | ||
| SS = np.zeros((ord_val, ord_val)) | ||
|
|
||
| for m in range(1, ord_val + 1): | ||
| B = np.zeros((nb_points, nb_incls), dtype=complex) | ||
| for i in range(nb_incls): | ||
| # cpoints = points[0] + 1j * points[1] | ||
| cpoints = inclusions[i].points[0, :] + 1j * inclusions[i].points[1, :] | ||
| dm = m * (cpoints**(m-1)) | ||
| toto = inclusions[i].normal[0, :] * dm + inclusions[i].normal[1, :] * dm * 1j | ||
| B[:, i] = toto | ||
|
|
||
| b = B.flatten(order='F') # Column-major flattening | ||
| if np.min(np.abs(lambda_val - 0.5)) < epsilon: | ||
| b_real = np.append(b.real, np.zeros(nb_incls)) | ||
| b_imag = np.append(b.imag, np.zeros(nb_incls)) | ||
| else: | ||
| b_real = b.real | ||
| b_imag = b.imag | ||
|
|
||
| # Solve Amat \ b | ||
| phi_m_real_vec = np.linalg.solve(Amat, b_real) if Amat.shape[0] == Amat.shape[1] else np.linalg.lstsq(Amat, b_real, rcond=None)[0] | ||
| phi_m_imag_vec = np.linalg.solve(Amat, b_imag) if Amat.shape[0] == Amat.shape[1] else np.linalg.lstsq(Amat, b_imag, rcond=None)[0] | ||
|
|
||
| # reshape back to (nb_points, nb_incls) | ||
| # We only take the first nb_points * nb_incls elements if we added extra rows | ||
| realphim = phi_m_real_vec[:nb_points*nb_incls].reshape((nb_points, nb_incls), order='F') | ||
| imagphim = phi_m_imag_vec[:nb_points*nb_incls].reshape((nb_points, nb_incls), order='F') | ||
|
|
||
| for n in range(1, ord_val + 1): | ||
| for i in range(nb_incls): | ||
| cpoints = inclusions[i].points[0, :] + 1j * inclusions[i].points[1, :] | ||
| zn = (cpoints**n) * inclusions[i].sigma | ||
|
|
||
| CC[m-1, n-1] += np.sum(zn.real * realphim[:, i].real) | ||
| CS[m-1, n-1] += np.sum(zn.imag * realphim[:, i].real) | ||
| SC[m-1, n-1] += np.sum(zn.real * imagphim[:, i].real) | ||
| SS[m-1, n-1] += np.sum(zn.imag * imagphim[:, i].real) |
There was a problem hiding this comment.
Le chemin fréquentiel perd toute la composante imaginaire du CGPT.
lambda_contrast() devient complexe dès que pmtt * freq != 0, mais ici les blocs CC/CS/SC/SS sont réels et l’accumulation tronque systématiquement realphim / imagphim avec .real. theoretical_CGPT() renverra donc une matrice artificiellement réelle pour les cas dispersifs.
Proposition de correctif
- CC = np.zeros((ord_val, ord_val))
- CS = np.zeros((ord_val, ord_val))
- SC = np.zeros((ord_val, ord_val))
- SS = np.zeros((ord_val, ord_val))
+ CC = np.zeros((ord_val, ord_val), dtype=complex)
+ CS = np.zeros((ord_val, ord_val), dtype=complex)
+ SC = np.zeros((ord_val, ord_val), dtype=complex)
+ SS = np.zeros((ord_val, ord_val), dtype=complex)
...
- CC[m-1, n-1] += np.sum(zn.real * realphim[:, i].real)
- CS[m-1, n-1] += np.sum(zn.imag * realphim[:, i].real)
- SC[m-1, n-1] += np.sum(zn.real * imagphim[:, i].real)
- SS[m-1, n-1] += np.sum(zn.imag * imagphim[:, i].real)
+ CC[m-1, n-1] += np.sum(zn.real * realphim[:, i])
+ CS[m-1, n-1] += np.sum(zn.imag * realphim[:, i])
+ SC[m-1, n-1] += np.sum(zn.real * imagphim[:, i])
+ SS[m-1, n-1] += np.sum(zn.imag * imagphim[:, i])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sies/asymp/__init__.py` around lines 117 - 156, The accumulation discards
imaginary parts by taking .real of realphim/imagphim and thus forces CC/CS/SC/SS
to be purely real when lambda_contrast() is complex; fix by keeping complex phi
fields and using the full complex arrays when computing zn * phi (i.e., do not
apply .real to realphim or imagphim, and compute contributions using complex
multiplication and then take real/imag parts only when assigning to CC/CS/SC/SS
as needed). Update the solve/results flow (phi_m_real_vec, phi_m_imag_vec ->
combine into complex phi_m_vec = phi_m_real_vec + 1j*phi_m_imag_vec, reshape
into complex realphim/imagphim or single complex phim) and use that complex phim
in the CC/CS/SC/SS accumulation; ensure any zero-padding logic for b_real/b_imag
preserves complex shape.
| def Green2D(X, Y): | ||
| """ | ||
| 2D Green function G(x) = 1/2/pi * log|x| evaluated at the points X-Y. | ||
| """ | ||
| X = np.asarray(X) | ||
| Y = np.asarray(Y) | ||
| XY1 = X[0, :, None] - Y[0, None, :] | ||
| XY2 = X[1, :, None] - Y[1, None, :] | ||
| DN = XY1**2 + XY2**2 | ||
| # G = 1/2/pi * log|x| = 1/4/pi * log|x|^2 | ||
| G = (1 / (4 * np.pi)) * np.log(DN) | ||
| return G |
There was a problem hiding this comment.
Attention : Green2D ne gère pas les points coïncidents.
Contrairement à Green2D_Grad (ligne 20 de laplacian/__init__.py), ta fonction Green2D ne protège pas contre DN == 0. Si X et Y ont des points coïncidents, np.log(0) retournera -inf avec un warning.
Pour la cohérence avec le reste de ton code, ajoute une protection similaire :
🛡️ Proposition de correction
DN = XY1**2 + XY2**2
+ # Handle zero distance (coincident points): set to small value to avoid log(0)
+ DN = np.where(DN == 0, np.finfo(float).tiny, DN)
# G = 1/2/pi * log|x| = 1/4/pi * log|x|^2
G = (1 / (4 * np.pi)) * np.log(DN)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sies/pde/base.py` around lines 43 - 54, Green2D currently calls np.log(DN)
and will produce -inf for coincident points (DN==0); modify Green2D to guard
against DN==0 before taking the log (e.g. replace zeros with a small positive
value or use np.where to set DN_safe = np.where(DN==0, eps, DN)) so np.log never
receives 0; refer to the local symbols DN and G in function Green2D and use a
stable epsilon like np.finfo(float).eps (or mirror the same handling used in
Green2D_Grad) so the function returns finite values without warnings.
| for n in range(N): | ||
| toto = Xs[:, n] - Z | ||
| R = np.linalg.norm(toto) | ||
| T = np.arctan2(toto[1], toto[0]) | ||
| for m in range(1, order + 1): | ||
| A[n, 2*(m-1):2*m] = [np.cos(m*T), np.sin(m*T)] / (2 * np.pi * m * R**m) | ||
| return A |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Résultat attendu : la première commande affiche un TypeError.
python - <<'PY'
import numpy as np
try:
[np.cos(0.1), np.sin(0.1)] / (2 * np.pi)
except Exception as exc:
print(type(exc).__name__, exc)
PY
# Localise l'affectation concernée dans le fichier revu.
rg -n "A\\[n, 2\\*\\(m-1\\):2\\*m\\]" sies/pde/conductivity.pyRepository: tomboulier/SIES
Length of output: 285
🏁 Script executed:
#!/bin/bash
# Check the actual file content around lines 149-155
sed -n '145,160p' sies/pde/conductivity.py
# Search for similar list division patterns
rg -n "\[.*\].*/" sies/pde/conductivity.py | head -20Repository: tomboulier/SIES
Length of output: 1017
🏁 Script executed:
#!/bin/bash
# Get complete function context to understand the issue better
sed -n '140,165p' sies/pde/conductivity.pyRepository: tomboulier/SIES
Length of output: 1043
🏁 Script executed:
#!/bin/bash
# Verify the TypeError with basic Python (no numpy needed)
python3 - <<'PY'
# Demonstrate that list division raises TypeError
try:
result = [1, 2] / 2
except TypeError as e:
print(f"TypeError confirmed: {e}")
# Show that numpy array division works
import sys
sys.path.insert(0, '/tmp')
# Demonstrate the fix
import numpy as np
arr = np.array([1.0, 2.0])
result = arr / 2
print(f"np.array division works: {result}")
PYRepository: tomboulier/SIES
Length of output: 267
Opération mathématique impossible sur une liste Python.
À la ligne 154, tu essaies de diviser une liste Python [np.cos(...), np.sin(...)] par un scalaire. En Python, les listes ne supportent pas la division : cela lèvera TypeError: unsupported operand type(s) for /: 'list' and 'int' dès le premier appel à reconstruct_CGPT().
Bonne pratique : quand on effectue une opération élément-par-élément sur des valeurs numériques, il faut utiliser un conteneur NumPy (array) plutôt qu'une liste Python. Cela garantit que les opérations arithmétiques fonctionnent comme prévu.
Correctif
- A[n, 2*(m-1):2*m] = [np.cos(m*T), np.sin(m*T)] / (2 * np.pi * m * R**m)
+ A[n, 2*(m-1):2*m] = np.array(
+ [np.cos(m * T), np.sin(m * T)]
+ ) / (2 * np.pi * m * R**m)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for n in range(N): | |
| toto = Xs[:, n] - Z | |
| R = np.linalg.norm(toto) | |
| T = np.arctan2(toto[1], toto[0]) | |
| for m in range(1, order + 1): | |
| A[n, 2*(m-1):2*m] = [np.cos(m*T), np.sin(m*T)] / (2 * np.pi * m * R**m) | |
| return A | |
| for n in range(N): | |
| toto = Xs[:, n] - Z | |
| R = np.linalg.norm(toto) | |
| T = np.arctan2(toto[1], toto[0]) | |
| for m in range(1, order + 1): | |
| A[n, 2*(m-1):2*m] = np.array( | |
| [np.cos(m * T), np.sin(m * T)] | |
| ) / (2 * np.pi * m * R**m) | |
| return A |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sies/pde/conductivity.py` around lines 149 - 155, The assignment is dividing
a Python list by a scalar (causing TypeError); change the RHS to a NumPy array
so elementwise division works and the shape matches the slice A[n, 2*(m-1):2*m];
specifically replace the list [np.cos(m*T), np.sin(m*T)] / (2 * np.pi * m *
R**m) with a NumPy array expression like np.array([np.cos(m*T), np.sin(m*T)]) /
(2 * np.pi * m * R**m) inside the loop (the issue occurs where A is assigned in
the loop that constructs A, e.g., in the function that contains the for n in
range(N) / for m in range(1, order+1) block).
| if symmode: | ||
| CGPT = (CGPT + CGPT.T) | ||
|
|
||
| out["CGPT"].append(CGPT) | ||
| residual = msr - op.L(CGPT, 'notransp').reshape((Ns, Nr), order='F') | ||
| out["res"].append(np.linalg.norm(residual, 'fro')) | ||
| out["rres"].append(out["res"][-1] / np.linalg.norm(msr, 'fro')) |
There was a problem hiding this comment.
Le résiduel est calculé contre 2M quand symmode=True.
Après symétrisation, CGPT représente déjà la matrice physique. Repasser ensuite par op.L(..., 'notransp') réapplique X + X.T et double la prédiction, ce qui fausse res et rres.
Proposition de correctif
- residual = msr - op.L(CGPT, 'notransp').reshape((Ns, Nr), order='F')
+ residual = msr - op.As @ CGPT @ op.Ar.T🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sies/pde/conductivity.py` around lines 233 - 239, Le calcul du résiduel
double la prédiction quand symmode est True car vous symétrisez CGPT (CGPT =
CGPT + CGPT.T) puis appelez op.L(..., 'notransp') qui réapplique X+X.T; corrigez
en conservant la version non symétrisée pour la prédiction : avant la ligne qui
fait CGPT = (CGPT + CGPT.T) sauvegardez CGPT_raw = CGPT, puis utilisez
op.L(CGPT_raw, 'notransp') pour calculer residual et les sorties out["res"] /
out["rres"] (ou alternativement appelez op.L(CGPT/2, 'notransp') si vous
préférez ne pas stocker CGPT_raw); référez-vous aux symboles symmode, CGPT,
CGPT_raw (nouveau), op.L, msr, out["res"] et out["rres"] pour localiser et
appliquer la correction.
| else: | ||
| CGPT = np.linalg.pinv(op.As) @ msr @ np.linalg.pinv(op.Ar.T) | ||
| out["CGPT"].append(CGPT) | ||
| residual = msr - op.As @ CGPT @ op.Ar.T | ||
| out["res"].append(np.linalg.norm(residual, 'fro')) | ||
| out["rres"].append(out["res"][-1] / np.linalg.norm(msr, 'fro')) |
There was a problem hiding this comment.
La branche pinv ignore complètement symmode.
La doc promet une reconstruction sous contrainte symétrique, mais ici method='pinv' renvoie la matrice brute sans projection ni solveur cohérent avec cette hypothèse. Les deux méthodes publiques n’ont donc pas la même sémantique.
Proposition de correctif minimal
else:
CGPT = np.linalg.pinv(op.As) @ msr @ np.linalg.pinv(op.Ar.T)
+ if symmode:
+ CGPT = 0.5 * (CGPT + CGPT.T)
out["CGPT"].append(CGPT)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sies/pde/conductivity.py` around lines 240 - 245, La branche method=='pinv'
calcule CGPT sans tenir compte de l'option symmode; corrigez-la pour appliquer
la même contrainte symétrique que l'autre méthode: calculez d'abord CGPT_raw =
np.linalg.pinv(op.As) @ msr @ np.linalg.pinv(op.Ar.T), puis si symmode est
activé projetez CGPT_raw sur l'espace symétrique (par ex. via
symmetrisation/averaging ou en appliquant la même fonction de projection
utilisée ailleurs) et affectez le résultat à CGPT avant d'ajouter out["CGPT"],
out["res"] et out["rres"]; modifiez la branche où apparaissent op.As, op.Ar,
CGPT et la clé method=='pinv' pour réutiliser la logique de projection symmode
existante afin d'unifier la sémantique des deux méthodes.
| p0_periodic = np.hstack([points0, points0[:, :1]]) | ||
| t0_periodic = np.append(theta0, theta0[-1] + (theta0[1]-theta0[0] if len(theta0)>1 else 2*np.pi)) | ||
|
|
There was a problem hiding this comment.
Le ré-échantillonnage n’interpole pas sur une période complète.
boundary_vec_interpl() ferme la spline avec theta0[-1] + pas, et rescale() requête ensuite seulement jusqu’à theta0[-1]. Quand dspl ne retombe pas exactement sur le dernier échantillon — par exemple 256 points avec dspl=10 — la pseudo-période devient 260/256 * 2π et le dernier arc de la courbe n’est jamais rééchantillonné. Les géométries générées ne sont donc plus exactement fermées.
Proposition de correctif
- t0_periodic = np.append(theta0, theta0[-1] + (theta0[1]-theta0[0] if len(theta0)>1 else 2*np.pi))
+ t0_periodic = np.append(theta0, theta0[0] + 2 * np.pi)
...
- theta = np.linspace(theta0[0], theta0[-1], nb_points, endpoint=False)
+ theta = np.linspace(theta0[0], theta0[0] + 2 * np.pi, nb_points, endpoint=False)Also applies to: 311-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sies/shape/base.py` around lines 253 - 255, La spline périodique est fermée
sur une pseudo-période mais rescale() ne demande les échantillons que jusqu’à
theta0[-1], ce qui laisse hors échantillonnage le dernier segment si dspl ne
divise pas exactement la période ; corrigez en faisant en sorte que la variable
périodique utilisée pour l’interpolation (ici t0_periodic) couvre la période
complète (inclure explicitement theta0[-1] + step_used) et assurez-vous que
rescale()/le code qui calcule les nouveaux theta demande des valeurs jusqu’à ce
t0_periodic[-1] (et non theta0[-1]) ; appliquez la même modification aux blocs
similaires référencés (les usages autour de p0_periodic/t0_periodic aux lignes
équivalentes 311-313) et garantissez que l’échantillonnage final prend en compte
ce dernier point périodique.
| def __mul__(self, s): | ||
| new_obj = super().__mul__(s) | ||
| new_obj.lside = self.lside * s | ||
| new_obj.angl = self.angl | ||
| return new_obj |
There was a problem hiding this comment.
Triangle.__mul__ renvoie en réalité un Shape, pas un Triangle.
super().__mul__() instancie Shape. Réattacher lside et angl sur cet objet ne restaure pas le type, donc l’API spécifique de Triangle est perdue après un redimensionnement.
Proposition de correctif
def __mul__(self, s):
- new_obj = super().__mul__(s)
- new_obj.lside = self.lside * s
- new_obj.angl = self.angl
- return new_obj
+ return Triangle(self.lside * s, self.angl, self.nb_points)Si dspl doit être conservé, stocke-le aussi dans __init__ et réutilise-le ici.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __mul__(self, s): | |
| new_obj = super().__mul__(s) | |
| new_obj.lside = self.lside * s | |
| new_obj.angl = self.angl | |
| return new_obj | |
| def __mul__(self, s): | |
| return Triangle(self.lside * s, self.angl, self.nb_points) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sies/shape/triangle.py` around lines 66 - 70, La méthode Triangle.__mul__
utilise super().__mul__() qui construit un Shape puis réattache lside/angl, ce
qui perd le type Triangle; changez __mul__ de façon à instancier et renvoyer un
vrai Triangle (copier lside, angl et tout autre attribut spécifique comme dspl)
au lieu d'utiliser super().__mul__(), et si dspl doit être conservé assurez-vous
qu'il est enregistré sur self dans __init__ puis copié dans le nouvel objet créé
par __mul__.
I have successfully translated the core of the SIES library from Matlab to Python.
Key Features:
shape + [x, y]for translation,shape * sfor scaling, andshape @ anglefor rotation.Conductivity_R2class is fully functional, supporting both forward data simulation and CGPT reconstruction (via LSQR or pseudo-inverse).examples/conductivity_demo.py) that replicates the conductivity simulation and reconstruction workflow.tests/test_sies.py) ensures the core functionality remains stable.pyproject.tomlconfiguration.You can run the demo notebook with
marimo run examples/conductivity_demo.pyor edit it withmarimo edit examples/conductivity_demo.py.PR created automatically by Jules for task 12105510887984873476 started by @tomboulier
Summary by CodeRabbit