Feature/variational model for out-of-plane bending#42
Merged
Conversation
…s and replace generalized decomposition function
…d energy release rate calculations and adjust segment handling for generalized backend
…ement and stress plots, and update energy release rate label for clarity
…ation selection and update documentation
…del attributes, and update Young's modulus calculations based on the selected model
…nd load vector attributes for external loading in mode III experiments
…segment is subject to additional loading or not
…_normal_tangential with decompose_to_xyz for load decomposition; update load handling in UnknownConstantsSolver based on segment loading state
…ants in WEAC simulation - Implemented the GeneralizedUnknownConstantsSolver class to compute unknown constants for the WEAC simulation. - Added methods for setting up boundary and transmission conditions for beam segments and weak layers. - Integrated logging for debugging and tracking the computation process. - Included error handling for linear system solving with detailed diagnostics.
…or correct scenario loading
…unknown constants solver logic
… correct rendering of rotation
- Introduced a new Jupyter notebook detailing the usage of the generalized backend for Weak Interface Avalanche Calculations (WEAC). - Included sections on installation, core components configuration, system model creation, result analysis, dynamic scenario updates, and visualization. - Provided examples and key API references to facilitate user understanding and implementation.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a generalized 24×24 eigensystem backend (eigensystem, field quantities, unknown-constants solver), propagates theta/loading flags and load vectors through configs, scenario, system selection and analysis, extends component models (layer, segment, slab), and updates plotting and utilities for generalized outputs. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as Config (backend="generalized")
participant SystemModel as SystemModel
participant Scenario as Scenario (theta, fi, gi)
participant GenEigen as GeneralizedEigensystem
participant GenSolver as GeneralizedUnknownConstantsSolver
participant GenFQ as GeneralizedFieldQuantities
participant Analyzer as Analyzer
Config->>SystemModel: init(config, slab, scenario)
SystemModel->>Scenario: refresh_from_config() (theta, load vectors)
Scenario->>Scenario: decompose_to_xyz(theta) -> qx,qy,qz
SystemModel->>GenEigen: init(weak_layer, slab)
GenEigen->>GenEigen: calc_eigensystem() (stiffness, K, eigenpairs)
SystemModel->>GenSolver: solve_for_unknown_constants(scenario, eigensystem)
GenSolver->>GenEigen: zh()/zp()/get_load_vector()
GenSolver->>GenSolver: assemble & solve Zh0 * C = rhs
GenSolver-->>SystemModel: return C
SystemModel->>GenFQ: init(eigensystem)
Analyzer->>SystemModel: rasterize_solution(theta, gi)
SystemModel->>GenEigen: z(x, C, phi, theta, is_loaded)
GenEigen-->>Analyzer: Z (24xN)
Analyzer->>GenFQ: Gi/Gii/Giii(Z_tip, Z_back, phi, theta)
GenFQ-->>Analyzer: ERR components (G_total, G_I, G_II, G_III)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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: 33
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demo/generalized_backend_documentation.ipynb`:
- Around line 412-416: The code is accessing a non-existent attribute
system.slab.h when building slab_thickness; update references to use the actual
Slab attribute name (system.slab.H) wherever slab_thickness is set or used
(e.g., the slab_thickness assignment and any later uses of slab_thickness),
ensuring consistency with the Slab class API (system.slab). Also scan nearby
slab-related lines (slab_width, slab_mass, slab_cog) to confirm they match the
Slab attribute names and adjust if any other attributes are misnamed.
In `@src/weac/analysis/analyzer.py`:
- Around line 652-670: The code builds identical cracked and uncracked states
because z_ct and z_ub are both created with the same segment/flags; update the
z_ub construction in the else-branch so it represents the unbroken side: when
computing z_ct use the current segment and true cracked flags (e.g.,
has_foundation=ki[idx], is_loaded=gi[idx]) and compute z_ub either from the
adjacent segment (e.g., use li[idx+1] / C[:,[idx+1]] when available) or by
flipping the crack/load booleans for the unbroken state (has_foundation=not
ki[idx], is_loaded=not gi[idx]); ensure you call self.sm.z(...) with those
distinct arguments for z_ct and z_ub before passing them into
self.sm.fq.Gi/Gii/Giii so Gi/Gii/Giii receive different cracked/uncracked
states.
In `@src/weac/analysis/plotter.py`:
- Around line 1692-1705: The two rotation traces in the generalized displacement
branch use identical LaTeX labels causing ambiguity; update the label strings
for the analyzer.sm.fq.psiy and analyzer.sm.fq.psiz entries so they are distinct
(for example change r"$\psi\ (^\circ)$ " to r"$\psi_y\ (^\circ)$" for psiy and
r"$\psi_z\ (^\circ)$" for psiz) so plots clearly differentiate ψy and ψz.
- Around line 1779-1783: incremental_ERR() can return a 3xN G, but the plotter
currently indexes G[3,...] and will IndexError; modify the code around the G
usage in plotter.py (the list using G[3], G[2], G[1], G[0]) to handle both 3-
and 4-row outputs: either pad G to 4 rows (e.g. G_padded = np.vstack([G,
np.zeros((1, G.shape[1]))]) when G.shape[0]==3) and then use G_padded in the
four-element list, or branch on G.shape[0] and build the list with the correct
row-to-label mapping for 3-row results; ensure you reference incremental_ERR(),
G, and the four-element list when making the change.
In `@src/weac/components/layer.py`:
- Line 235: Fix the typo in the Field description for the constitutive_model
attribute in src/weac/components/layer.py by replacing "interlniked" with
"interlinked", and reformat the long line so it adheres to line length limits
(e.g., split the Field(...) call across multiple lines or wrap the description
string) while preserving the Literal type and default value for
constitutive_model.
- Around line 184-186: Docstring unit for parameter f is inconsistent with the
actual computation (rho*1e-12*G_MM_S2) which yields N/mm^3; update the docstring
for f (and any duplicate doc entries) to state "Resultant force of the layer
[N/mm^3]" so it matches the field description "Weight density of the weak layer
[N/mm^3]" and the conversion using G_MM_S2; locate the docstring that documents
f and change its unit annotation accordingly.
- Around line 270-280: Local variable names use camelCase (nuUpdate, EUpdate);
rename them to snake_case (nu_update, e_update) throughout the method in
src/weac/components/layer.py: replace nuUpdate→nu_update and EUpdate→e_update in
the branches for constitutive_model and in the subsequent object.__setattr__
calls (ensure object.__setattr__(self, "nu", nu_update) and
object.__setattr__(self, "E", e_update)); update any other local references in
the same function/method to maintain consistency.
- Line 285: The assignment currently overwrites legitimate f=0.0 because it uses
a falsy check; change the attribute default to use a sentinel (e.g., f:
Optional[float] = None) and in the initializer/model_post_init compute f using
an explicit None check: set self.f to self.rho*1e-12*G_MM_S2 only when f is
None, otherwise keep the provided value (including 0.0). Update any places that
assume truthiness of f (the line with object.__setattr__(self, "f", ...), and
code in model_post_init) to use "is None" instead of truthiness.
In `@src/weac/components/scenario_config.py`:
- Around line 23-24: In the docstring for the theta parameter in
scenario_config.py, change the parameter line from "theta float, optional" to
"theta : float, optional" to match the numpydoc style used for phi; update the
theta description accordingly so the signature reads "theta : float, optional"
followed by the existing explanation about rotation of the slab
(counterclockwise positive).
- Around line 93-103: The commented-out validator check_load_vector_shape for
load_vector_left and load_vector_right is dead code; either remove it or
re-enable it to enforce shape (6,1). If you need validation, uncomment the
`@field_validator`("load_vector_left", "load_vector_right", mode="after") and the
check_load_vector_shape function, ensure it imports numpy as np and returns the
reshaped np.asarray value (raising ValueError if not length 6), and add tests;
otherwise delete the entire commented block to avoid confusion (also review
Segment.f for similar commented validators and apply the same decision).
- Around line 5-8: The imports Any and field_validator are unused; remove them
to clean up the module: delete Any from the typing import and drop
field_validator from the pydantic import list (leave BaseModel, Field,
ConfigDict intact). If the commented validator will be re-enabled instead,
restore usage by uncommenting the validator and referencing field_validator
accordingly; otherwise remove the unused imports.
In `@src/weac/components/segment.py`:
- Around line 48-51: The current field validator ensure_ndarray (decorated with
`@field_validator`("f")) only coerces dtype but must also enforce that the
resulting array has shape (6, 1); update ensure_ndarray to convert the input to
a numpy array with dtype float, then check its shape and either reshape a (6,)
vector to (6,1) or raise a ValueError if the shape is incompatible (use the same
acceptance logic as the commented validator in scenario_config.py for guidance).
Ensure the error message identifies the field "f" and expected shape "(6, 1)" so
downstream code using the Segment class fails fast on invalid input.
- Around line 5-8: Consolidate the pydantic imports in segment.py by combining
BaseModel, Field, field_validator, and ConfigDict into a single from pydantic
import ... statement (replace the two separate pydantic import lines). Ensure
references to BaseModel, Field, field_validator, and ConfigDict remain unchanged
in the file so functionality is preserved.
In `@src/weac/core/field_quantities.py`:
- Around line 137-141: Remove the commented-out old expression and add
parentheses to clarify operator precedence in the return expression: replace the
current return that uses self._unit_factor(unit) * self.es.weak_layer.kn / 2 *
self.w(Ztip)**2 (and the commented-out self.sig(...) line) with a single line
using explicit grouping, e.g. self._unit_factor(unit) * (self.es.weak_layer.kn /
2) * (self.w(Ztip) ** 2), ensuring you delete the commented-out old formula
(self.sig(...) ...) and keep references to self._unit_factor,
self.es.weak_layer.kn and self.w(Ztip) intact.
In `@src/weac/core/generalized_eigensystem.py`:
- Around line 159-324: Many coefficient definitions (e.g., c0201, c0213, c0403,
c0412, c0803, c1001, c1204, c1406, c1803, c2403, etc.) reuse very large
denominator expressions which makes the matrix hard to review and test; extract
each distinct repeated denominator (and large repeated numerator sub-expressions
where sensible) into well-named variables (for example denom_elastic = <shared
-4*D11... + 12*B11**2*Pi**2...>, denom_plate = <b**2*((-6+Pi**2)*h*Ew +
6*kA55*Pi**2*(1+nuw))**2 - 18*Pi**2*(1+nuw)*...>, denom_shear =
<(-1+2*nuw)*((-6+Pi**2)*h*Ew + 6*kA55*Pi**2*(1+nuw))>, etc.) and replace usages
in all coefficients (c0201..c2423) so each coefficient reads as a short
expression like numerator/denom_elastic; add a small internal function or dict
to collect these denominators for unit tests and create a short verification
notebook or test that evaluates each named denom and asserts consistency with a
symbolic reference (or checks numeric equality across random valid parameter
draws) to catch sign/term mistakes.
- Around line 613-624: get_load_vector currently ignores the has_foundation
flag; update get_load_vector to branch on has_foundation and compute the load
vector differently when False by omitting foundation reaction contributions
(i.e., treat foundation-related lever arms/forces as zero) instead of always
using the foundation-based z_s/y_s values. In practice, inside get_load_vector
check has_foundation and for False set z_s and y_s (and any qs-derived moment
terms) to 0 so my, mz, mx are computed without foundation reactions; keep the
existing computation for True. Reference symbols: get_load_vector,
has_foundation, z_s, y_s, z_w, qw_x/qw_y/qw_z, qs_x/qs_y/qs_z, my, mz, mx.
- Around line 673-676: The q construction in generalized_eigensystem (variable
q) has syntax/semantic errors: insert the missing commas between [q03] and [q04]
and between [q17] and [q18] so each entry is a separate list element (e.g.,
...,[q03],[q04],... and ...,[q17],[q18],...), and remove the trailing comma
after the np.array(...) call so q is an ndarray (not a one-element tuple);
ensure dtype=np.double remains inside the np.array(...) call.
- Around line 338-371: Update the docstring to state the system_matrix is 24x24
(not 6x6) and replace the exact floating-point imaginary comparison with a
tolerance-based check: compute ew, ev = np.linalg.eig(system_matrix) as before,
then use a small tolerance (e.g. tol = 1e-8) and classify real eigenvalues with
real = np.isclose(ew.imag, 0, atol=tol) & (np.abs(ew.real) > tol) and complex
conjugate pairs with cmplx = ew.imag > tol; keep the rest of the logic that
assigns ewC, ewR, evC, evR, sR, sC unchanged but now using these tolerance-based
masks to avoid misclassification of numerically-real eigenvalues.
- Line 16: The file imports unused symbols Optional and G_MM_S2; remove the
unused import of Optional from the typing import and remove the G_MM_S2 import
so they are not imported into generalized_eigensystem.py (check the import lines
where Optional and G_MM_S2 appear and delete them), and if any type annotations
relied on Optional replace with built-in typing (e.g., use X | None) or adjust
annotations accordingly in functions/classes like those in this module.
In `@src/weac/core/generalized_field_quantities.py`:
- Around line 290-332: There are large commented-out reference implementations
inside Gi, Gii, and Giii that clutter the code; remove these commented blocks or
move them to a separate documentation file/notebook for verification, and keep
the functions Gi, Gii, Giii implementations concise; if you must retain the
reference formulas in the repo, extract them into a new markdown or Jupyter
notebook (or a clearly named docstring constant) and add a short one-line
comment in each function (Gi, Gii, Giii) pointing to that file so readers know
where the original expressions live.
- Around line 141-175: Docstrings for several theta accessors were copy-pasted
and incorrectly describe the physical direction; update them to match the
quantity each function returns: change theta_vc to "constant amplitude of
out-of-plane cosine shaped displacements in the weak layer", theta_vl to "linear
amplitude of out-of-plane cosine shaped displacements in the weak layer",
theta_wc to "constant amplitude of vertical cosine shaped displacements in the
weak layer", and theta_wl to "linear amplitude of vertical cosine shaped
displacements in the weak layer" (leave theta_ul as axial); also ensure the
corresponding derivative methods (dtheta_vc_dx, dtheta_vl_dx, dtheta_wc_dx,
dtheta_wl_dx) have matching corrected docstrings; locate these by the function
names theta_ul, dtheta_ul_dx, theta_vc, dtheta_vc_dx, theta_vl, dtheta_vl_dx,
theta_wc, dtheta_wc_dx, theta_wl in generalized_field_quantities.py and replace
the incorrect "axial" text with the appropriate "out-of-plane" or "vertical"
wording.
- Around line 86-92: Docstring in dw_dx has a stray "(*" causing an unmatched
parenthesis; update the docstring for the dw_dx method to match the intended
description (e.g., """First derivative w' = w₀' + b₀ ψx'."""), removing the
stray characters and ensuring consistency with the w method's docstring style;
edit the dw_dx function definition to replace the current docstring with the
corrected text.
- Around line 334-345: The dz_dx and dz_dxdx methods call
self.es.get_load_vector missing the required theta argument; update both method
signatures to accept theta (e.g., def dz_dx(self, z: np.ndarray, phi: float,
theta: float, qs: float = 0)) and pass that theta through to get_load_vector
(get_load_vector(phi=phi, theta=theta, qs=qs,...)). Also propagate theta through
all downstream callers that invoke dz_dx/dz_dxdx—specifically update du0_dxdx,
dpsi_dxdx, du0_dxdxdx, and dpsi_dxdxdx to accept a theta parameter and forward
it into dz_dx/dz_dxdx and any direct get_load_vector calls so signatures and
calls remain consistent.
- Around line 256-260: The function sig_zz uses a falsy check "if not h0" which
treats 0.0 as "no value"; change the parameter sentinel to None and guard with
"if h0 is None" so an explicit 0.0 is respected: update the signature of sig_zz
to accept h0: float | None = None (instead of bool | float = False) and replace
the conditional with "if h0 is None: h0 = self.es.slab.H/2 +
self.es.weak_layer.h/2"; keep the rest of the computation unchanged and ensure
any type hints and callers are adjusted to accept None as the default.
In `@src/weac/core/generalized_unknown_constants_solver.py`:
- Around line 85-87: The code fills pi with "m" using np.full then assigns pi[0]
= "length", which silently truncates to "l" due to the 1-char dtype; update the
assignment to explicitly set the left boundary to "l" (i.e., replace pi[0] =
"length" with pi[0] = "l") in the block around the np.full(...) and pi[...]
assignments so the intent is explicit and robust even if dtype semantics change.
- Line 16: Remove the invalid and unused import "from scipy.sparse.linalg import
eigen" from the module; locate the import statement in
generalized_unknown_constants_solver.py (the line importing
scipy.sparse.linalg.eigen) and delete it so the module no longer raises
ImportError on import and unused-import warnings are eliminated.
In `@src/weac/core/scenario.py`:
- Around line 194-221: The method _calc_normal_load currently computes only the
z-components (qwz, qsz) and sets self.qz but never computes or sets the
y-component (self.qy), leaving rotated slabs inconsistent; call decompose_to_xyz
for qw and qs to capture the middle (y) components (e.g., _, qwy, _ =
decompose_to_xyz(qw, phi, theta) and _, qsy, _ = decompose_to_xyz(qs, phi,
theta)), sum them into qy = qwy + qsy and assign self.qy = qy (mirroring how qx
is set elsewhere).
- Around line 171-176: The bug is np.append is flattening self.fi when adding a
dummy segment; instead append a new row with shape (1,6) using an axis-aware
operation. Replace the np.append(self.fi, np.zeros((6,1))) call with an axis=0
concatenate (e.g. self.fi = np.concatenate([self.fi, np.zeros((1,6))], axis=0)
or self.fi = np.vstack([self.fi, np.zeros((1,6))])) and keep the other appends
(self.li, self.ki, self.mi) consistent (or use np.concatenate with matching
shapes) so self.fi ends up with shape (n,6) not flattened.
- Around line 117-119: The code sets self.load_vector_right incorrectly to
self.scenario_config.load_vector_left, overwriting the configured right vector;
change the assignment so that self.load_vector_right =
self.scenario_config.load_vector_right (keep self.load_vector_left =
self.scenario_config.load_vector_left as-is) to respect the configured right
load vector in the Scenario initialization.
In `@src/weac/core/slab.py`:
- Around line 66-68: The Slab constructor currently accepts b without
validation; add a guard in Slab.__init__ that checks b > 0 and raises a
ValueError (with a clear message like "slab width b must be > 0") if not, so any
later divisions by self.b in methods of Slab are safe; update the __init__
signature in class Slab to validate and set self.b only after the check.
In `@src/weac/core/system_model.py`:
- Line 138: The update_layers method rebuilds the Slab without carrying forward
the configured width, overriding the b value set in __init__ (where Slab is
constructed with config.b); modify update_layers so when it constructs a new
Slab it passes the existing slab width (use self.slab.b) into the Slab
constructor (e.g., Slab(layers=new_layers, b=self.slab.b)) to preserve the
configured slab width across updates.
In `@src/weac/utils/misc.py`:
- Around line 14-18: The function decompose_to_xyz lacks a return type
annotation; add an explicit return type that reflects it returns an (x, y, z)
triple of the same scalar/array types as the inputs, e.g. ->
tuple[NDArray[np.float64] | float, NDArray[np.float64] | float,
NDArray[np.float64] | float]; update imports/type hints if your Python version
requires typing.Tuple or typing.Union instead of the PEP 604 union operator.
- Around line 45-47: The f_y component computation in src/weac/utils/misc.py is
missing the cos(phi) factor causing magnitude and boundary-condition errors;
update the assignment for f_y (the variable used alongside f_x and f_z in the
same block) to include the cos(phi) factor so it matches f_z's structure (i.e.,
compute f_y as f * cos(phi) * sin(theta)), ensuring the vector magnitude is
preserved and the phi=90° case yields zero cross-slope component.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demo/generalized_backend_documentation.ipynb`:
- Around line 450-457: The code directly mutates system.scenario.scenario_config
(setting .phi and .theta) which is fragile if ScenarioConfig becomes immutable;
instead construct a new ScenarioConfig instance (copying existing values and
changing phi/theta) and pass that new instance into
system.update_scenario(scenario_config=...) so you never mutate
system.scenario.scenario_config in place; reference the existing ScenarioConfig
type/constructor (or a .copy()/replace() helper if available) and use
system.update_scenario to accept the fresh instance (similar to how immutable
WeakLayer objects are handled).
- Around line 144-149: The example passes E=None which violates WeakLayer.E's
pydantic type (float, ge=0, default 0.0) and causes ValidationError; update the
example to either omit the E argument or set E=0.0 so model_post_init can
auto-calc the modulus. Locate the WeakLayer instantiation in the notebook and
replace E=None with either no E parameter or E=0.0 and ensure any explanatory
text mentions that 0.0 triggers auto-calculation in WeakLayer.model_post_init.
- Around line 418-429: Update the misleading inline comment for weak_layer.f to
reflect that weak_layer.f is the resultant force of the layer (units N/mm)
rather than a friction coefficient, and clarify that kB55 and kD55 are
attributes of the GeneralizedEigensystem (not present on the classic
Eigensystem) — since this notebook uses the generalized backend, keep A11, B11,
D11, kA55, kB55, kD55 as shown but add a brief note or parenthetical after kB55
and kD55 indicating “(GeneralizedEigensystem-only)” so readers won’t expect
those attributes on the classic Eigensystem.
In `@src/weac/analysis/analyzer.py`:
- Line 19: Remove the unused top-level import "from weac.core import scenario"
from analyzer.py; the module is not referenced directly (code uses
self.sm.scenario), so delete that import to eliminate the unused symbol and
clean up imports in the file.
- Around line 773-774: Tests still reference the old Scenario attributes qn/qt
which were renamed to qz/qx; update the tests to use scenario.qz and scenario.qx
instead of qn/qt (search for qn and qt usages in the tests, e.g. the assertions
in tests/core/test_scenario.py) and run the test suite to confirm no
AttributeError remains. Ensure any mock or fixture creating Scenario instances
uses the new attribute names as well so references to qn/qt are fully replaced.
In `@src/weac/components/config.py`:
- Around line 46-48: The b field definition in config.py is missing its closing
parenthesis and uses an unnecessary f-string prefix; fix the Field declaration
for the variable named b by replacing f"Out-of-plane width..." with a plain
string and adding the missing closing ')' (and a trailing comma) so the next
field forced_touchdown_mode: TouchdownMode | None = Field(...) parses correctly;
ensure the Field call for b is properly closed before the forced_touchdown_mode
definition.
In `@src/weac/core/slab.py`:
- Line 138: The comment for the qw calculation is incorrect: the expression qw =
sum(rhoi * G_MM_S2 * hi) still yields a line load in N/mm, not N/mm/mm; either
revert the comment back to "Line load [N/mm]" or change the computation/usage to
represent a load per unit area by dividing by b (or multiplying by b where
used). Locate the qw assignment in slab.py (variable qw using rhoi, G_MM_S2, hi)
and update either the comment to the original "Line load [N/mm]" or adjust the
formula to qw = sum(rhoi * G_MM_S2 * hi) / b (and verify downstream consumers
expect N/mm/mm) so units are consistent.
- Around line 64-67: The __init__ signature uses the undefined typing name
List[Layer], causing a NameError and inconsistency with the class attribute
which uses list[Layer]; fix by making the types consistent: either import List
from typing at the top and keep List[Layer] or (preferred) change the __init__
parameter annotation to use built-in generic syntax list[Layer] to match the
class attribute and Python 3.9+ style (update the signature of __init__ in
slab.py accordingly and ensure Layer is referenced the same way).
In `@src/weac/core/system_model.py`:
- Around line 160-168: The return type of the eigensystem cached_property is
incorrect—eigensystem can return either Eigensystem or GeneralizedEigensystem—so
update its annotation to reflect both types (e.g., use Union[Eigensystem,
GeneralizedEigensystem] or the PEP 604 syntax Eigensystem |
GeneralizedEigensystem) similar to the fq property pattern; ensure you import
typing.Union or adjust imports for the new annotation and update the type on the
eigensystem method definition.
- Around line 447-475: The z() implementation duplicates the same concat/dot
structure across four branches differing only by whether eigensystem.zp(...,
theta, ...) is called; refactor to compute the common part once and call
eigensystem.zp with a conditional argument list: build a helper that for a
single xi returns np.dot(self.eigensystem.zh(xi, length, has_foundation), C) +
self.eigensystem.zp(xi, phi, *( [theta] if self.config.backend=="generalized"
else [] ), has_foundation, qs if is_loaded else 0.0) (or equivalent handling for
scalar vs iterable), then use that helper in the list comprehension (and for
scalar) to remove duplicated branches while preserving behavior of z(),
eigensystem.zh, eigensystem.zp, config.backend, is_loaded, qs, phi, theta.
- Line 19: Remove the unused and shadowing import "from pydantic import config"
at the top of the module; it conflicts with the local parameter/name "config"
used throughout the SystemModel class (and related functions/methods) so simply
delete that import and run linters/tests to ensure no other references to
pydantic.config remain.
In `@src/weac/core/unknown_constants_solver.py`:
- Around line 22-23: Remove the stale commented-out import line that references
G_MM_S2 and LSKI_MM; delete the commented code so only active imports remain
(e.g., keep from weac.utils.misc import get_skier_point_load, decompose_to_xyz)
and ensure no other code depends on the removed symbols G_MM_S2 or LSKI_MM in
unknown_constants_solver.py (check for usages and remove or replace if present).
- Around line 119-121: The long inline conditional in the calls to
eigensystem.zp (for zl and zr) makes lines too long and hard to format; compute
an intermediate variable (e.g., effective_surface_load = qs if is_loaded else 0)
before the zl and zr calls and then pass effective_surface_load as the qs
argument to eigensystem.zp (referencing zl, zr, eigensystem.zp, is_loaded, qs)
to shorten the lines and satisfy formatting checks.
- Line 205: The code is using Cartesian loads (scenario.qx) but tests expect
slope-relative properties (qn, qt); add slope-relative properties to Scenario
and switch usage accordingly: implement or update
decompose_to_normal_tangential() to compute and set self.qn and self.qt (using
the existing _calc_normal_load and _calc_tangential_load computations or by
transforming qx,qy,qz into normal/tangential components), ensure
_calc_normal_load() and _calc_tangential_load() set the correct attributes
(qn/qt or delegate appropriately), and replace the use of scenario.qx in
unknown_constants_solver.py (N = ...) with scenario.qt so the solver uses the
tangential load.
---
Duplicate comments:
In `@src/weac/analysis/analyzer.py`:
- Around line 686-706: The bug is that z_ct and z_ub are built identically so
Gi/Gii/Giii see no state difference; change the z construction so the cracked
and uncracked states differ: when computing z_ct / z_ub with self.sm.z (and used
by self.sm.fq.Gi, Gii, Giii) ensure one represents the cracked case (e.g.,
has_foundation=False or using the cracked segment constants) and the other
represents the unbroken/foundation-supported case (e.g., has_foundation=True or
using the adjacent segment's C[:,[idx+1]] when available); update the z_ct/z_ub
calls accordingly and keep the existing phi, theta, unit, gi[idx], qs usage
consistent (use a fallback to C[:,[idx]] if idx+1 is out of range).
In `@src/weac/analysis/plotter.py`:
- Around line 2060-2073: The legend labels for rotations are ambiguous because
analyzer.sm.fq.psiy and analyzer.sm.fq.psiz both use the same string r"$\psi\
(^\circ)$ "; update the label for psiy and psiz to distinct, descriptive LaTeX
strings (e.g. r"$\psi_y\ (^\circ)$" and r"$\psi_z\ (^\circ)$" or similar with
subscripts) in the data list used by the plotting logic in plotter.py so the
plot legend clearly differentiates psiy from psiz.
- Around line 2141-2159: plot_ERR_modes currently assumes G has 4 rows and
unconditionally indexes G[3, :], which crashes for classic backends that return
3 rows; change plot_ERR_modes to check G.shape[0] and build ax1data
conditionally: if G.shape[0] == 4 keep the existing 4-entry list (using G[3],
G[2], G[1], G[0] with the same labels), but if G.shape[0] == 3 construct a
3-entry list using [da/10, 1e3*G[2,:], label+"$_\\mathrm{I\\!I}$"], [da/10,
1e3*G[1,:], label+"$_\\mathrm{I}$"], and [da/10, 1e3*G[0,:],
label+"$_\\mathrm{I+I\\!I}$"] (or adjust the total/combined label to reflect
classic totals), then pass that conditional data into self._plot_data (leave
other args unchanged) so no IndexError occurs when G has only 3 rows.
In `@src/weac/components/layer.py`:
- Line 287: Update the Field description for the constitutive_model attribute to
fix the typo: change "interlniked" to "interlinked" in the description string
for constitutive_model (the Field(...) attached to the constitutive_model
Literal["PlaneStrain","PlaneStress","Uniaxial"] declaration).
- Around line 233-235: The docstring for parameter `f` in the Layer component is
inconsistent with the actual computation (rho*1e-12*G_MM_S2) which produces
units of N/mm^3; update the docstring and any field description to use N/mm^3 so
they match the computation (e.g., change "Resultant force of the layer [N/mm]"
to "Resultant force of the layer [N/mm^3]"), and ensure related names (`f`,
`rho`, and constant `G_MM_S2`) and the Field description for the weak layer all
reflect the same N/mm^3 units.
- Around line 322-332: Replace the camelCase locals nuUpdate and EUpdate with
snake_case names (e.g., nu_update and e_update or nu_eff and e_eff) in the
branch that computes updates for self.nu and self.E based on
self.constitutive_model; update their assignments in the 'PlaneStrain',
'PlaneStress', and 'Uniaxial' branches and use those snake_case names in the
subsequent object.__setattr__(self, "nu", ...) and object.__setattr__(self, "E",
...) calls so the code uses Python naming conventions consistently while still
assigning the computed values to self.nu and self.E.
- Line 337: The assignment for attribute f uses a falsy-check ("self.f or ...")
which unintentionally overwrites valid zero values; update the logic to treat
None as the sentinel instead (e.g., set f's default to None and change the
assignment in the constructor to only use the fallback when self.f is None),
ensuring you modify the place that currently does object.__setattr__(self, "f",
...) and any constructor signature or default where f is defined so that zero is
preserved.
In `@src/weac/components/scenario_config.py`:
- Around line 5-8: Remove the unused imports to clean up the module: delete the
imported symbols Any and field_validator from the top-level import line so only
actually used names (e.g., Literal, BaseModel, Field, ConfigDict, numpy as np)
remain; if you plan to re-enable the commented validator later, restore
field_validator at that time, otherwise remove it now to avoid lint warnings.
- Around line 95-105: The commented-out validator for
load_vector_left/load_vector_right (check_load_vector_shape) must be reinstated
or removed; restore and enable it by uncommenting and registering it with the
Pydantic validator decorator for those fields, implement converting the input to
a numpy array (np.asarray(..., dtype=np.float64)), reshape to (-1,1) and
validate the first dimension equals 6, and raise a ValueError if not; reference
the existing function name check_load_vector_shape and the fields
load_vector_left/load_vector_right so the fix is applied to that validator (or,
if you choose to remove dead code, delete the commented block entirely).
- Around line 25-26: In the docstring for the slab rotation parameter, change
the malformed "theta float, optional" to the correct numpydoc style "theta :
float, optional" (mirroring how `phi` is documented) so the parameter section
parses correctly; update the `theta` entry in scenario_config.py's docstring
accordingly.
In `@src/weac/core/field_quantities.py`:
- Around line 141-145: Remove the commented-out legacy expression and make
operator precedence explicit: replace the current return so it no longer
contains the commented line and use parentheses around the weak_layer.kn
division (e.g. use (self.es.weak_layer.kn / 2)) and around the full product if
helpful; target the return statement that uses self._unit_factor(unit),
self.es.weak_layer.kn and self.w(Ztip)**2 to implement this clarity in the
method containing that return.
In `@src/weac/core/scenario.py`:
- Around line 171-176: The code that pads single-segment scenarios uses
np.append on self.fi which flattens the 2D array; update the padding for self.fi
to preserve its 2D shape (e.g., use np.append with axis=0 or use
np.vstack/np.concatenate) so appended zeros have shape (1,6) and the resulting
self.fi remains (n,6); adjust the other appends only if necessary but keep
references to self.li, self.ki, self.mi, and self.fi and ensure the shapes you
append match each array's dimensionality.
- Around line 117-118: The assignment mistakenly sets self.load_vector_right to
scenario_config.load_vector_left (copy-paste error); change the right-hand side
to self.scenario_config.load_vector_right so that self.load_vector_right is
populated from the correct config property (update the assignment in the
Scenario class where load_vector_left/load_vector_right are initialized).
- Around line 194-221: The bug is that self.qy is never assigned because the
y-component returned by decompose_to_xyz is ignored; update
_calc_tangential_load (and if desired _calc_normal_load for completeness) to
capture and sum the y-components from decompose_to_xyz for qw and qs and assign
the result to self.qy. Specifically, in _calc_tangential_load capture the middle
return values (e.g., qwy and qsy) from decompose_to_xyz(qw, phi, theta) and
decompose_to_xyz(qs, phi, theta), compute qy = qwy + qsy and set self.qy = qy so
accesses to self.qy won't raise AttributeError. Ensure you reference
decompose_to_xyz, _calc_tangential_load, self.qy, qw and qs when making the
change.
In `@src/weac/core/slab.py`:
- Around line 64-66: Validate the slab width parameter in Slab.__init__: check
the incoming b and raise a ValueError (with a clear message) if b is <= 0 before
assigning to self.b so downstream divisions by self.b cannot cause
divide-by-zero or invalid results; keep the check near the top of __init__
(before setting self.b) and preserve existing layers assignment.
In `@src/weac/core/system_model.py`:
- Around line 333-343: The update_layers method recreates a Slab with the
default width (b=300) and thus drops any configured slab width; modify
update_layers to pass the existing slab width into the new Slab (e.g.,
Slab(layers=new_layers, b=self.slab.b) or, if self.slab may be None, fall back
to self.scenario.slab.b or self.config.b) so the new Scenario keeps the original
slab width when you recreate self.slab and self.scenario.
Flo-rhein
changed the title
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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 `@src/weac/analysis/plotter.py`:
- Line 2260: The labelpos calculation was moved to the center (labelpos =
int(0.5 * len(x[~np.isnan(x)]))) which shifts labels away from the right edge
and breaks staggered placement for 3–5 series; change it to pick a position near
the right end of the valid x values (e.g., compute valid_idx =
np.where(~np.isnan(x))[0] and then use either
valid_idx[int(0.95*len(valid_idx))] or valid_idx[-1] as labelpos) and clamp the
staggered placement (the expression labelpos - 10*i) so it never falls before
valid_idx[0] (use max(valid_idx[0], labelpos - 10*i)). This preserves right-end
placement for both classic (3-series) and generalized (up to 5-series) cases
while keeping labels legible.
- Around line 2094-2099: In plot_stresses() the code negates
analyzer.sm.fq.tau_xz(z, unit="kPa") but not analyzer.sm.fq.tau_yz(z,
unit="kPa"), which is inconsistent because tau_xz and tau_yz are defined
identically; either negate both or neither and add a brief inline comment
documenting the sign convention. Locate the three-element rows built in
plot_stresses() (the list containing tau_xz, tau_yz, sig_zz), make the tau_xz
and tau_yz entries consistent (both prefixed with a minus or neither), and add a
short comment like "# Negated to match sign convention" referencing the choice;
ensure changes align with generalized_field_quantities.py behavior for
tau_xz/tau_yz.
---
Duplicate comments:
In `@src/weac/analysis/plotter.py`:
- Around line 2146-2151: The code assumes G has four rows (uses G[3, :]) but the
classic backend's incremental_ERR() can return a 3×N array, causing IndexError;
modify the data construction in plotter.py to check G.shape[0] (or query the
backend) before accessing G[3, :], and only append the fourth-row entry (the one
using G[3, :] and label + r"$_\mathrm{I\!I\!I}$") when G.shape[0] >= 4;
otherwise build the list using the available rows (e.g., indices 0..2) or adjust
labels accordingly so no out-of-range indexing occurs (refer to the
variables/data structure named data, G, and label and the producer function
incremental_ERR or backend selector to determine row count).
Flo-rhein
commented
Feb 17, 2026
Flo-rhein
left a comment
Flo-rhein
left a comment
Collaborator
Author
There was a problem hiding this comment.
Checked all code rabbit comments
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/weac/analysis/analyzer.py (1)
583-624:⚠️ Potential issue | 🟠 MajorPropagate
is_loadedflags inincremental_ERR().
Currentlyz_uncracked/z_crackedalways useis_loaded=True(default), so segments withgi=Falsestill getqsapplied. This can skew ERR for partially loaded PST scenarios.🛠️ Suggested fix
- li = self.sm.scenario.li - ki = self.sm.scenario.ki + li = self.sm.scenario.li + ki = self.sm.scenario.ki + gi = self.sm.scenario.gi @@ - C_uncracked, C_cracked, li = ( + C_uncracked, C_cracked, li, gi = ( C_uncracked[:, iscrack], C_cracked[:, iscrack], - li[iscrack], + li[iscrack], + gi[iscrack], ) @@ z_uncracked = partial( self.sm.z, C=C_uncracked[:, [j]], length=length, phi=phi, theta=theta, has_foundation=True, + is_loaded=gi[j], qs=qs, ) z_cracked = partial( self.sm.z, C=C_cracked[:, [j]], length=length, phi=phi, theta=theta, has_foundation=False, + is_loaded=gi[j], qs=qs, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/weac/analysis/analyzer.py` around lines 583 - 624, The incremental_ERR logic currently always uses the default is_loaded=True when building z_uncracked/z_cracked, so segments that are not loaded (gi=False) still receive qs; fix by reading gi = self.sm.scenario.gi near the top, apply the same reduction iscrack = k0 & ~ki to slice gi (e.g., gi = gi[iscrack]), and then pass the per-segment flag into the partials as is_loaded=gi[j] for both z_uncracked and z_cracked so unloaded segments do not get qs applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demo/generalized_backend_documentation.ipynb`:
- Around line 313-317: The example incorrectly references an undefined variable
config_generalized when constructing the SystemModel; update the
SystemModel(...) call to pass the existing config_no_touchdown instead (i.e.,
replace config_generalized with config_no_touchdown) so
SystemModel(model_input=model_input, config=config_no_touchdown) uses the
defined configuration.
In `@src/weac/core/generalized_eigensystem.py`:
- Around line 496-500: Add an inline comment near the computation of my in zp
(the line computing my = qw_x * z_w + qs_x * z_s) explaining that the sign
difference between this expression and the one computed in get_load_vector
(where my is negated) is intentional and mathematically compensated by the
opposite coefficient signs (e.g. zp uses (-2*my + ...) while get_load_vector
uses (2*my + ...)), and note that mx and mz remain unchanged; place the comment
next to the my assignment in the zp routine (referencing my, zp,
get_load_vector, mx, mz) so future readers understand the sign convention and
equivalence.
In `@src/weac/core/generalized_field_quantities.py`:
- Around line 256-260: The h0 parameter in sig_zz is being overwritten
unconditionally because the guard is inverted; change the signature and check so
callers can rely on passing h0: make h0 default to None (e.g., Optional[float])
and replace the current condition with if h0 is None: then set h0 =
self.es.slab.H/2 + self.es.weak_layer.h/2; keep the rest of sig_zz logic and
update the type annotation from bool | float to a nullable float type to reflect
the sentinel correctly.
- Line 12: The import G_MM_S2 in generalized_field_quantities.py is unused;
remove the unused import statement to clean up the module and avoid lint
warnings by deleting the token G_MM_S2 from the from weac.constants import ...
line (or removing that import line entirely if it contained only G_MM_S2) and
run tests/lint to confirm no references remain.
- Around line 189-223: The Vy, Vz, Mx, My and Mz methods have their
has_foundation branches inverted: when has_foundation is True they currently
return only the slab term and when False they add the weak-layer correction;
invert the if/else in each of these methods so that has_foundation=True returns
slab + weak_layer correction and has_foundation=False returns the slab-only
expression (same pattern as Nx); update the bodies of Vy, Vz, Mx, My, and Mz
accordingly, keeping the existing weak-layer corrective expressions but moving
them into the has_foundation=True branch.
In `@src/weac/core/generalized_unknown_constants_solver.py`:
- Around line 60-64: The docstring's Returns section is stale: it documents the
solution matrix C as "Matrix(6xN)" but the solver uses nDOF_supported = 24 and
builds C_return with shape (nS, 24) before transpose, so update the Returns
description to "Matrix(24xN)" (or otherwise reflect 24 rows of constants per
segment) and mention that columns contain the 24 constants per segment; ensure
the variable names C and C_return and nDOF_supported are referenced consistently
in the text.
- Around line 23-24: Remove the dead/commented import and the two unused
imports: delete the commented-out import line for weac.constants (the commented
G_MM_S2 line), remove the G_MM_S2 symbol from any import statements, and remove
get_skier_point_load from the from weac.utils.misc import list so only used
symbols (e.g., decompose_to_xyz) remain; update the import statement in
generalized_unknown_constants_solver to import only decompose_to_xyz.
- Around line 576-605: The branch in _setup_conditions_weak_layer uses a bare if
for the "r"/"right" case which can leave conditions uninitialized; change that
if to an elif to match the other position branches (so the pos checks are
mutually exclusive) and add a final else clause that either assigns a safe
default to conditions or raises a clear ValueError for an unexpected pos; ensure
the variable conditions (and the logger.debug call referencing it) is always
defined.
---
Outside diff comments:
In `@src/weac/analysis/analyzer.py`:
- Around line 583-624: The incremental_ERR logic currently always uses the
default is_loaded=True when building z_uncracked/z_cracked, so segments that are
not loaded (gi=False) still receive qs; fix by reading gi = self.sm.scenario.gi
near the top, apply the same reduction iscrack = k0 & ~ki to slice gi (e.g., gi
= gi[iscrack]), and then pass the per-segment flag into the partials as
is_loaded=gi[j] for both z_uncracked and z_cracked so unloaded segments do not
get qs applied.
---
Duplicate comments:
In `@demo/generalized_backend_documentation.ipynb`:
- Around line 416-418: The documentation lists kB55 and kD55 without noting they
are only present on the generalized eigensystem; update the comment around the
kB55/kD55 lines to state that kB55 and kD55 are attributes of
GeneralizedEigensystem only (and not available on the classic eigensystem),
referencing system.eigensystem and GeneralizedEigensystem so readers know which
class provides these shear-stiffness attributes.
- Around line 144-149: The example sets WeakLayer(E=None) which violates the
model constraint that WeakLayer.E is a non-negative float; remove the E
parameter from the WeakLayer(...) call (or set E=0.0 if an explicit value is
needed) so the instantiation uses the class default or a valid float; update the
example block where WeakLayer is constructed to omit E or use E=0.0 and run the
example to verify no validation error is raised.
- Around line 407-409: The comment incorrectly states that system.weak_layer.f
is a friction coefficient; update the code around the weak_layer_f binding to
reflect that f is the weak-layer weight density/force per volume: either rename
the variable from weak_layer_f to weak_layer_weight_density (or
weak_layer_force_density) and update any downstream uses, or keep weak_layer_f
but change the inline comment to "weight density / force per volume" so it is
accurate; also verify weak_layer_nu comment remains "Poisson's ratio" for
system.weak_layer.nu.
- Around line 439-446: The code mutates the existing ScenarioConfig via
system.scenario.scenario_config, which can leak changes to other references;
instead construct a fresh ScenarioConfig (e.g., by copying or using a
constructor/clone/replace helper on the ScenarioConfig type) and modify fields
on that new instance (set phi and theta) and pass it to
system.update_scenario(scenario_config=new_config) so
system.scenario.scenario_config is not mutated in place.
In `@src/weac/analysis/plotter.py`:
- Around line 2258-2284: The current label placement in the loop over ax1data
(using labelpos and the per-series offset formula labelpos - 10 * i) can push
indices out of bounds and crowd labels; modify the code in the block that builds
xtx/ytx for each series (the loop over ax1data where i is incremented and xtx =
(x[labelpos - 10 * i - 1] + x[labelpos - 10 * i]) / 2 etc.) to compute a base
index near the right end (e.g., use min(len(x)-1, labelpos) or a right_end =
max(0, len(x)-1 - right_padding)), apply a per-series offset that is clamped
within [1, len(x)-1], and use those clamped indices when computing xtx and ytx;
ensure the same clamping is applied in both the FEA/empty-label branch
(thislabelpos) and the normal branch to avoid negative or OOB indexing.
In `@src/weac/components/layer.py`:
- Around line 233-235: The docstring for WeakLayer.f is incorrect: it lists the
type and units as "float" and "N/mm" while the attribute is typed as "float |
None" and represents a computed quantity in "N/mm^3". Update the WeakLayer class
docstring so the entry for f reads something like "f : float | None — Resultant
force of the layer [N/mm^3]; None if not yet computed" (and ensure any adjacent
param like nu remains accurate), making the optionality and correct units
explicit.
In `@src/weac/core/generalized_eigensystem.py`:
- Around line 15-24: Remove the unused imports to satisfy linting: delete
Optional from the typing import and remove G_MM_S2 from the weac.constants
import list in generalized_eigensystem.py (i.e., update the lines that import
Optional and the tuple containing SHEAR_CORRECTION_FACTOR and G_MM_S2 so only
used symbols remain).
- Around line 673-676: The assignment to q in generalized_eigensystem.py
currently ends with a trailing comma causing q to be a one-element tuple instead
of an ndarray; remove the trailing comma after the np.array(...) call so q is
the ndarray expected by downstream callers (look for the q = np.array([...],
dtype=np.double), line near the TODO comment and return q and ensure q remains a
numpy.ndarray before returning).
- Around line 613-624: get_load_vector currently ignores the has_foundation flag
and always builds the foundation-based load vector; update get_load_vector to
branch on has_foundation and compute the correct load vector for each case.
Specifically, inside get_load_vector use the existing decompose_to_xyz calls
(qw_x,qw_y,qw_z; qs_x,qs_y,qs_z; f_x,f_y,f_z) but if has_foundation is False
compute z_w, z_s, y_s and the moments (my,mz,mx) using the non-foundation
formula (i.e., omit foundation contribution / use slab geometry for free
segments) and if True keep the existing foundation-based math; ensure the TODO
is removed and callers relying on has_foundation get the proper branch.
- Around line 358-371: The code currently classifies eigenvalues using exact
checks (ew.imag == 0 and ew.imag > 0) which misclassifies nearly-real values;
update the classification in generalized_eigensystem.py to use a tolerance
(e.g., tol = 1e-12 or tol = np.finfo(float).eps * (1 + np.abs(ew))) and replace
real = (ew.imag == 0) & (ew.real != 0) with real = (np.abs(ew.imag) <= tol) &
(ew.real != 0) and cmplx = ew.imag > tol (or cmplx = np.abs(ew.imag) > tol) so
ewC, ewR, evC, evR, sR, sC are computed from robust boolean masks; ensure tol is
defined near where ew is available and used consistently for both
positive/complex classification.
In `@src/weac/core/generalized_field_quantities.py`:
- Around line 141-175: Update the incorrect docstrings: for theta_vc and
theta_vl replace "axial" with "out-of-plane", for theta_wc and theta_wl replace
"axial" with "vertical", and change theta_uc's docstring from "Linear amplitude"
to "Constant amplitude" to reflect the `c`/`l` suffix semantics; also verify
associated derivative methods dtheta_vc_dx, dtheta_vl_dx, dtheta_wc_dx have
matching direction and constant/linear wording and correct them if they don't
(refer to theta_vc, theta_vl, theta_wc, theta_wl, theta_uc and their dtheta_*
counterparts).
- Around line 290-332: The Gi, Gii and Giii methods contain large blocks of
commented-out alternative implementations that clutter the code; remove those
commented blocks from each function and either (a) extract the commented
formulas into a separate verification file/notebook (e.g., a docs or notebooks
entry) with a short header referencing the original methods Gi, Gii, Giii, or
(b) delete them if no longer needed, while keeping the current active return
lines and behavior intact; commit the extracted content as a separate file so
the git history still preserves the old code if you need to recover it.
- Around line 334-345: dz_dx and dz_dxdx currently call
self.es.get_load_vector(phi=phi, qs=qs) but get_load_vector requires theta;
update both methods to accept a theta parameter and pass it through (e.g.,
get_load_vector(phi=phi, theta=theta, qs=qs, ...)). Also update all downstream
derivative helpers that call these methods—du0_dxdx, dpsi_dxdx, du0_dxdxdx,
dpsi_dxdxdx—to accept a theta argument and forward it when calling dz_dx/dz_dxdx
and get_load_vector so theta is propagated through the entire derivative chain.
Ensure any optional has_foundation or other kwargs required by get_load_vector
are forwarded as needed.
In `@src/weac/core/generalized_unknown_constants_solver.py`:
- Around line 84-86: The code uses pi = np.full(nS, "m") which creates a
fixed-length string dtype and silently truncates "length" to "l"; update the
initialization of pi to avoid truncation by creating an object- or unicode-typed
array (e.g., use np.full(nS, "m", dtype=object) or dtype='<U6') or initialize
with a Python list and convert to ndarray so that assigning pi[0] = "length" and
pi[-1] = "r" preserves the full strings; change the np.full call in
generalized_unknown_constants_solver.py (symbol: pi and np.full) accordingly.
In `@src/weac/core/scenario.py`:
- Around line 194-201: The code calls decompose_to_xyz(qw, phi, theta) and
decompose_to_xyz(qs, phi, theta) in _calc_tangential_load/_calc_normal_load but
discards the y-components, leaving self.qy unset; update these methods to
capture the second return value (e.g., qwy and qsy) and assign self.qy = qwy +
qsy (or the appropriate combination) alongside self.qx so downstream consumers
can read self.qy without AttributeError; ensure you reference the
decompose_to_xyz calls and the self.qx assignment in scenario.py when making the
change.
In `@src/weac/core/slab.py`:
- Around line 64-68: The type name List used in Slab.__init__ is undefined;
update the annotation to use the built-in generic by changing the parameter type
from List[Layer] to list[Layer] in the Slab.__init__ signature (or alternatively
add "from typing import List" at the module top); ensure you only modify the
annotation in the Slab.__init__ method that references Layer so the NameError is
resolved.
- Line 140: The comment for qw is inconsistent: qw = sum(rhoi * G_MM_S2 * hi)
computes a line load in N/mm (not N/mm/mm); either change the comment back to
"N/mm" or, if you intended a per-width value, divide the sum by the slab width
variable b. Update the comment or modify the expression for qw accordingly and
ensure references to qw, rhoi, G_MM_S2, hi and b are used so the fix is easy to
locate.
In `@src/weac/core/system_model.py`:
- Around line 333-336: update_layers currently creates a new Slab from
new_layers but drops the configured width, reverting to the Slab default; modify
update_layers (method name update_layers in the class that owns self.slab) to
construct the new Slab with the existing slab width (e.g.,
width=self.slab.width) when self.slab is present so the slab width is preserved,
otherwise fall back to the default width; ensure you assign the new Slab back to
self.slab.
Comment on lines
+313
to
+317
| "# Create the system model with generalized backend\n", | ||
| "system = SystemModel(\n", | ||
| " model_input=model_input,\n", | ||
| " config=config_generalized,\n", | ||
| ")\n", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "generalized_backend_documentation.ipynb" -type f 2>/dev/null | head -5Repository: 2phi/weac
Length of output: 35
🏁 Script executed:
fd "generalized_backend_documentation.ipynb"Repository: 2phi/weac
Length of output: 97
🏁 Script executed:
cat "demo/generalized_backend_documentation.ipynb" | python3 << 'EOF'
import json
import sys
# Read notebook
data = json.load(sys.stdin)
# Extract all cells
cells = data.get('cells', [])
# Look for variable definitions and the problematic cell
for i, cell in enumerate(cells):
if cell.get('cell_type') == 'code':
source = ''.join(cell.get('source', []))
# Search for config definitions
if 'config_generalized' in source or 'config_no_touchdown' in source or 'SystemModel' in source:
print(f"\n=== Cell {i} ===")
print(source[:500])
EOFRepository: 2phi/weac
Length of output: 773
🏁 Script executed:
python3 << 'EOF'
import json
import re
# Read notebook from file
with open('demo/generalized_backend_documentation.ipynb', 'r') as f:
try:
data = json.load(f)
except json.JSONDecodeError as e:
print(f"JSON Error: {e}")
exit(1)
# Extract all cells and look for variable definitions
cells = data.get('cells', [])
print(f"Total cells: {len(cells)}\n")
# Track config definitions and SystemModel usage
config_defs = {}
system_model_usage = []
for i, cell in enumerate(cells):
if cell.get('cell_type') == 'code':
source = ''.join(cell.get('source', []))
# Look for config_generalized definition
if 'config_generalized' in source:
print(f"--- Cell {i} (config_generalized found) ---")
print(source)
print()
# Look for config_no_touchdown definition
if 'config_no_touchdown' in source:
print(f"--- Cell {i} (config_no_touchdown found) ---")
print(source)
print()
# Look for SystemModel instantiation
if 'SystemModel(' in source:
print(f"--- Cell {i} (SystemModel found) ---")
print(source)
print()
EOFRepository: 2phi/weac
Length of output: 2609
🏁 Script executed:
python3 << 'EOF'
import json
with open('demo/generalized_backend_documentation.ipynb', 'r') as f:
data = json.load(f)
cells = data.get('cells', [])
found_any_definition = False
for i, cell in enumerate(cells):
if cell.get('cell_type') == 'code':
source = ''.join(cell.get('source', []))
# Look for definition pattern: config_generalized =
if 'config_generalized' in source and '=' in source:
lines = source.split('\n')
for line in lines:
if 'config_generalized' in line and '=' in line and not line.strip().startswith('#'):
print(f"Cell {i}: {line}")
found_any_definition = True
if not found_any_definition:
print("No definition of 'config_generalized' found in the notebook")
EOFRepository: 2phi/weac
Length of output: 92
config_generalized is undefined in the SystemModel example.
Cell 19 uses config_generalized but only config_no_touchdown is defined earlier, causing a NameError. Replace with config_no_touchdown.
🔧 Suggested fix
system = SystemModel(
model_input=model_input,
- config=config_generalized,
+ config=config_no_touchdown,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@demo/generalized_backend_documentation.ipynb` around lines 313 - 317, The
example incorrectly references an undefined variable config_generalized when
constructing the SystemModel; update the SystemModel(...) call to pass the
existing config_no_touchdown instead (i.e., replace config_generalized with
config_no_touchdown) so SystemModel(model_input=model_input,
config=config_no_touchdown) uses the defined configuration.
Comment on lines
+496
to
+500
| y_s = 0 | ||
| # Clean up code by adapting mx,my,mz and f_x,f_y,f_z | ||
| my = qw_x * z_w + qs_x * z_s | ||
| mz = -qs_x * y_s | ||
| mx = -qw_y * z_w + qs_z * y_s - qs_y * z_s |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/weac/core/generalized_eigensystem.py | sed -n '490,510p'Repository: 2phi/weac
Length of output: 2274
🏁 Script executed:
cat -n src/weac/core/generalized_eigensystem.py | sed -n '615,630p'Repository: 2phi/weac
Length of output: 1509
🏁 Script executed:
cat -n src/weac/core/generalized_eigensystem.py | sed -n '470,530p'Repository: 2phi/weac
Length of output: 5721
🏁 Script executed:
cat -n src/weac/core/generalized_eigensystem.py | sed -n '595,650p'Repository: 2phi/weac
Length of output: 5940
🏁 Script executed:
cd src/weac/core && grep -n "moment\|sign\|convention" generalized_eigensystem.py | head -20Repository: 2phi/weac
Length of output: 35
🏁 Script executed:
find . -name "*.md" -o -name "*.rst" -o -name "*.txt" | xargs grep -l "sign\|moment\|convention" 2>/dev/null | head -10Repository: 2phi/weac
Length of output: 74
🏁 Script executed:
find . -name "*test*.py" -o -name "test_*" -type f | head -10Repository: 2phi/weac
Length of output: 399
🏁 Script executed:
cat -n ./tests/core/test_eigensystem.pyRepository: 2phi/weac
Length of output: 15546
🏁 Script executed:
cat README.mdRepository: 2phi/weac
Length of output: 22917
🏁 Script executed:
grep -n "2\*my\|4\*my" src/weac/core/generalized_eigensystem.py | head -20Repository: 2phi/weac
Length of output: 2406
🏁 Script executed:
# Check if there's any documentation or comments about sign conventions
grep -B5 -A5 "my = " src/weac/core/generalized_eigensystem.pyRepository: 2phi/weac
Length of output: 854
🏁 Script executed:
grep -n "2\*mx\|4\*mx" src/weac/core/generalized_eigensystem.py | head -10Repository: 2phi/weac
Length of output: 4903
Document the intentional sign compensation for my between zp and get_load_vector.
The sign difference in my is mathematically compensated: in zp (line 498), my = qw_x * z_w + qs_x * z_s is used with coefficient (-2*my + ...), while in get_load_vector (line 621), my = -qw_x * z_w - qs_x * z_s is used with coefficient (2*my + ...). Both expand to the same expression. Since mx and mz remain identical in both methods, clarify this pattern with an inline comment explaining the sign convention to improve code readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/weac/core/generalized_eigensystem.py` around lines 496 - 500, Add an
inline comment near the computation of my in zp (the line computing my = qw_x *
z_w + qs_x * z_s) explaining that the sign difference between this expression
and the one computed in get_load_vector (where my is negated) is intentional and
mathematically compensated by the opposite coefficient signs (e.g. zp uses
(-2*my + ...) while get_load_vector uses (2*my + ...)), and note that mx and mz
remain unchanged; place the comment next to the my assignment in the zp routine
(referencing my, zp, get_load_vector, mx, mz) so future readers understand the
sign convention and equivalence.
| import numpy as np | ||
|
|
||
| from weac.core.generalized_eigensystem import GeneralizedEigensystem | ||
| from weac.constants import G_MM_S2 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify G_MM_S2 is not referenced in the file beyond the import
rg -n "G_MM_S2" src/weac/core/generalized_field_quantities.pyRepository: 2phi/weac
Length of output: 90
Remove unused import G_MM_S2 — this constant is imported but never referenced in the module.
Proposed fix
-from weac.constants import G_MM_S2📝 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.
Suggested change
| from weac.constants import G_MM_S2 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/weac/core/generalized_field_quantities.py` at line 12, The import G_MM_S2
in generalized_field_quantities.py is unused; remove the unused import statement
to clean up the module and avoid lint warnings by deleting the token G_MM_S2
from the from weac.constants import ... line (or removing that import line
entirely if it contained only G_MM_S2) and run tests/lint to confirm no
references remain.
Comment on lines
+189
to
+223
| def Vy(self, Z: np.ndarray, has_foundation: bool) -> float | np.ndarray: | ||
| """Vertical out-of-plane shear force Vy = kA66 b (-psiz + v')- kB66 b psix' [N]""" | ||
| if has_foundation: | ||
| return self.es.slab.b*self.es.kA55*(-self.psiz(Z) + Z[3,:]) - self.es.slab.b*self.es.kB55*self.dpsix_dx(Z) | ||
| else: | ||
| return self.es.slab.b*self.es.kA55*(-self.psiz(Z) + Z[3,:]) - self.es.slab.b*self.es.kB55*self.dpsix_dx(Z) + (self.es.weak_layer.h*self.es.weak_layer.E*(12*self.theta_ul(Z) + self.es.slab.b*(-2*np.pi*self.psiz(Z) + 2*np.pi*Z[3,:] + 6*self.dtheta_vc_dx(Z) - self.es.slab.H*np.pi*self.dpsix_dx(Z))))/(12*np.pi*(1 + self.es.weak_layer.nu)) | ||
|
|
||
| def Vz(self, Z: np.ndarray, has_foundation: bool) -> float | np.ndarray: | ||
| """Vertical shear force V = kA55(w' + psiy) [N]""" | ||
| if has_foundation: | ||
| return self.es.slab.b*self.es.kA55*(self.psiy(Z) + Z[5,:]) | ||
| else: | ||
| return self.es.slab.b*self.es.kA55*(self.psiy(Z) + Z[5,:]) + (self. | ||
| es.slab.b*self.es.weak_layer.E*(-6*np.pi*Z[0,:] + 24*self.theta_uc(Z) - 3*self.es.slab.H*np.pi*self.psiy(Z) + 4*np.pi*self.es.weak_layer.h*Z[5,:] + 12*self.es.weak_layer.h*self.dtheta_wc_dx(Z)))/(24*np.pi*(1 + self.es.weak_layer.nu)) | ||
|
|
||
| def Mx(self, Z: np.ndarray, has_foundation: bool) -> float | np.ndarray: | ||
| """Torsional moment Mx = kA55 * b^3/12 * psix' + kB66 b (psiz - v0') + kD66 b psix' in the slab [Nmm]""" | ||
| if has_foundation: | ||
| return self.es.slab.b*self.es.kB55*(self.psiz(Z) - Z[3,:]) + (self.es.slab.b**3*self.es.kA55*self.dpsix_dx(Z))/12 + self.es.slab.b*self.es.kD55*self.dpsix_dx(Z) | ||
| else: | ||
| return self.es.slab.b*self.es.kB55*(self.psiz(Z) - Z[3,:]) + (self.es.slab.b**3*self.es.kA55*self.dpsix_dx(Z))/12 + self.es.slab.b*self.es.kD55*self.dpsix_dx(Z) - (self.es.slab.b**2*self.es.weak_layer.E*(-24*self.es.weak_layer.h*self.theta_ul(Z) + 3*self.es.slab.b*np.pi*(-2 + self.es.weak_layer.h)*self.psiz(Z) + 12*(-2 + self.es.weak_layer.h)*self.es.weak_layer.h*self.dtheta_wl_dx(Z) + self.es.slab.b*np.pi*(-3 + self.es.weak_layer.h)*self.es.weak_layer.h*self.dpsix_dx(Z)))/(144*np.pi*self.es.weak_layer.h*(1 + self.es.weak_layer.nu)) - (self.es.slab.H*self.es.weak_layer.h*self.es.weak_layer.E*(12*self.theta_ul(Z) + self.es.slab.b*(-2*np.pi*self.psiz(Z) + 2*np.pi*Z[3,:] + 6*self.dtheta_vc_dx(Z) - self.es.slab.H*np.pi*self.dpsix_dx(Z))))/(24*np.pi*(1 + self.es.weak_layer.nu)) | ||
|
|
||
| def My(self, Z: np.ndarray, has_foundation: bool) -> float | np.ndarray: | ||
| """Bending moment My = B11 b u' + D11 b psiy' in the slab[Nmm]""" | ||
| if has_foundation: | ||
| return self.es.slab.b*self.es.B11*Z[1,:] + self.es.slab.b*self.es.D11*self.dpsiy_dx(Z) | ||
| else: | ||
| return self.es.slab.b*self.es.B11*Z[1,:] + self.es.slab.b*self.es.D11*self.dpsiy_dx(Z) + (self.es.slab.H*self.es.weak_layer.E*(3*self.es.slab.b*np.pi*self.es.weak_layer.nu*Z[4,:] - 12*self.es.weak_layer.h*self.es.weak_layer.nu*self.theta_vl(Z) - 12*self.es.slab.b*self.es.weak_layer.nu*self.theta_wc(Z) + self.es.slab.b*self.es.weak_layer.h*(-1 + self.es.weak_layer.nu)*(2*np.pi*Z[1,:] + 6*self.dtheta_uc_dx(Z) + self.es.slab.H*np.pi*self.dpsiy_dx(Z))))/(12*np.pi*(-1 + self.es.weak_layer.nu + 2*self.es.weak_layer.nu**2)) | ||
|
|
||
| def Mz(self, Z: np.ndarray, has_foundation: bool) -> float | np.ndarray: | ||
| """Bending moment Mz = A11 b^3/12 * psiz' in the slab [Nmm]""" | ||
| if has_foundation: | ||
| return (self.es.A11*self.es.slab.b**3*self.dpsiz_dx(Z))/12 | ||
| else: | ||
| return (self.es.A11*self.es.slab.b**3*self.dpsiz_dx(Z))/12 - (self.es.slab.b**2*self.es.weak_layer.E*(-24*self.es.weak_layer.nu*self.theta_wl(Z) + 3*self.es.slab.b*np.pi*self.es.weak_layer.nu*self.psix(Z) + 2*self.es.weak_layer.h*(-1 + self.es.weak_layer.nu)*(6*self.dtheta_ul_dx(Z) - self.es.slab.b*np.pi*self.dpsiz_dx(Z))))/(72*np.pi*(-1 + self.es.weak_layer.nu + 2*self.es.weak_layer.nu**2)) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
sed -n '180,225p' src/weac/core/generalized_field_quantities.pyRepository: 2phi/weac
Length of output: 4845
Swap has_foundation branches in Vy, Vz, Mx, My, and Mz — weak-layer corrections are inverted.
Nx (line 180) correctly returns slab + weak_layer when has_foundation=True and slab only when False. The five methods below reverse this: they return slab only for supported segments and add the weak-layer correction for unsupported ones.
| Method | has_foundation=True |
has_foundation=False |
|---|---|---|
| Nx (line 180) | slab + weak_layer ✓ | slab ✓ |
| Vy (line 189) | slab | slab + weak_layer |
| Vz (line 196) | slab | slab + weak_layer |
| Mx (line 204) | slab | slab + weak_layer |
| My (line 211) | slab | slab + weak_layer |
| Mz (line 218) | slab | slab + weak_layer |
For unsupported segments, the weak-layer DOFs (Z[12:]–Z[23]) are zero, causing the extra terms to vanish without visible error. For supported segments, the weak-layer correction is silently omitted, producing incorrect section forces.
Fix: swap if/else branches for all five methods
def Vy(self, Z: np.ndarray, has_foundation: bool) -> float | np.ndarray:
"""Vertical out-of-plane shear force Vy = kA66 b (-psiz + v')- kB66 b psix' [N]"""
if has_foundation:
- return self.es.slab.b*self.es.kA55*(-self.psiz(Z) + Z[3,:]) - self.es.slab.b*self.es.kB55*self.dpsix_dx(Z)
+ return self.es.slab.b*self.es.kA55*(-self.psiz(Z) + Z[3,:]) - self.es.slab.b*self.es.kB55*self.dpsix_dx(Z) + (self.es.weak_layer.h*self.es.weak_layer.E*(12*self.theta_ul(Z) + self.es.slab.b*(-2*np.pi*self.psiz(Z) + 2*np.pi*Z[3,:] + 6*self.dtheta_vc_dx(Z) - self.es.slab.H*np.pi*self.dpsix_dx(Z))))/(12*np.pi*(1 + self.es.weak_layer.nu))
else:
- return self.es.slab.b*self.es.kA55*(-self.psiz(Z) + Z[3,:]) - self.es.slab.b*self.es.kB55*self.dpsix_dx(Z) + (self.es.weak_layer.h*self.es.weak_layer.E*(12*self.theta_ul(Z) + self.es.slab.b*(-2*np.pi*self.psiz(Z) + 2*np.pi*Z[3,:] + 6*self.dtheta_vc_dx(Z) - self.es.slab.H*np.pi*self.dpsix_dx(Z))))/(12*np.pi*(1 + self.es.weak_layer.nu))
+ return self.es.slab.b*self.es.kA55*(-self.psiz(Z) + Z[3,:]) - self.es.slab.b*self.es.kB55*self.dpsix_dx(Z)Apply the same swap pattern to Vz, Mx, My, and Mz.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/weac/core/generalized_field_quantities.py` around lines 189 - 223, The
Vy, Vz, Mx, My and Mz methods have their has_foundation branches inverted: when
has_foundation is True they currently return only the slab term and when False
they add the weak-layer correction; invert the if/else in each of these methods
so that has_foundation=True returns slab + weak_layer correction and
has_foundation=False returns the slab-only expression (same pattern as Nx);
update the bodies of Vy, Vz, Mx, My, and Mz accordingly, keeping the existing
weak-layer corrective expressions but moving them into the has_foundation=True
branch.
Comment on lines
+23
to
+24
| # from weac.constants import G_MM_S2, LSKI_MM | ||
| from weac.utils.misc import decompose_to_xyz, get_skier_point_load |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "G_MM_S2|get_skier_point_load" --type=py src/weac/core/generalized_unknown_constants_solver.py | grep -v "^.*:.*import"Repository: 2phi/weac
Length of output: 35
🏁 Script executed:
head -30 src/weac/core/generalized_unknown_constants_solver.py | cat -nRepository: 2phi/weac
Length of output: 1199
Remove unused imports and dead code.
Lines 18, 23-24 contain unused imports: G_MM_S2 is imported but never used; get_skier_point_load is imported but never used; and line 23 has a commented-out import that should be removed. Remove line 18, line 23 entirely, and get_skier_point_load from line 24.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/weac/core/generalized_unknown_constants_solver.py` around lines 23 - 24,
Remove the dead/commented import and the two unused imports: delete the
commented-out import line for weac.constants (the commented G_MM_S2 line),
remove the G_MM_S2 symbol from any import statements, and remove
get_skier_point_load from the from weac.utils.misc import list so only used
symbols (e.g., decompose_to_xyz) remain; update the import statement in
generalized_unknown_constants_solver to import only decompose_to_xyz.
Comment on lines
+576
to
+605
| if pos in ("r", "right"): | ||
| bcs = cls._boundary_conditions_weak_layer( | ||
| zr, | ||
| eigensystem, | ||
| pos, | ||
| system_type, | ||
| ) # Right boundary conditions | ||
| conditions = np.array( | ||
| [ | ||
| -fq.theta_uc(zl), # -theta_uci(xi = 0) | ||
| -fq.theta_ul(zl), # -theta_uli(xi = 0) | ||
| -fq.theta_vc(zl), # -theta_vci(xi = 0) | ||
| -fq.theta_vl(zl), # -theta_vli(xi = 0) | ||
| -fq.theta_wc(zl), # -theta_wci(xi = 0) | ||
| -fq.theta_wl(zl), # -theta_wli(xi = 0) | ||
| -fq.Nx_c_weakLayer(zl), # -Nx_c_weak_layeri(xi = 0) | ||
| -fq.Nx_l_weakLayer(zl), # -Nx_l_weak_layeri(xi = 0) | ||
| -fq.Vy_c_weakLayer(zl), # -Vy_c_weak_layeri(xi = 0) | ||
| -fq.Vy_l_weakLayer(zl), # -Vy_l_weak_layeri(xi = 0) | ||
| -fq.Vz_c_weakLayer(zl), # -Vz_c_weak_layeri(xi = 0) | ||
| -fq.Vz_l_weakLayer(zl), # -Vz_l_weak_layeri(xi = 0) | ||
| bcs[0], | ||
| bcs[1], | ||
| bcs[2], | ||
| bcs[3], | ||
| bcs[4], | ||
| bcs[5], | ||
| ] | ||
| ) | ||
| logger.debug("Boundary Conditions at pos %s: %s", pos, conditions.shape) # pylint: disable=E0606 |
Contributor
There was a problem hiding this comment.
if on Line 576 should be elif — matches the pattern in _setup_conditions_slab and guards against conditions being unbound.
The _setup_conditions_slab method uses elif for all branches, but _setup_conditions_weak_layer switches to a bare if for the "r"/"right" case. While functionally harmless today (the positions are mutually exclusive), if pos is none of the expected values, conditions is never assigned and Line 605 raises UnboundLocalError.
Proposed fix
- if pos in ("r", "right"):
+ elif pos in ("r", "right"):And add an else guard:
+ else:
+ raise ValueError(f"Unknown position: {pos!r}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/weac/core/generalized_unknown_constants_solver.py` around lines 576 -
605, The branch in _setup_conditions_weak_layer uses a bare if for the
"r"/"right" case which can leave conditions uninitialized; change that if to an
elif to match the other position branches (so the pos checks are mutually
exclusive) and add a final else clause that either assigns a safe default to
conditions or raises a clear ValueError for an unexpected pos; ensure the
variable conditions (and the logger.debug call referencing it) is always
defined.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/weac/core/system_model.py (1)
253-278:⚠️ Potential issue | 🟠 MajorGeneralized solver silently ignores touchdown parameters — guard against unsupported combination.
When
config.backend == "generalized"andconfig.touchdown == True, the code selectsGeneralizedUnknownConstantsSolverand passes touchdown parameters (touchdown_distance,touchdown_mode,collapsed_weak_layer_kR), but the generalized solver does not implement touchdown logic. This silently produces incorrect results. The docstring at line 75 already notes touchdown is "only available for the classical model", but the code path doesn't enforce this.Consider raising an error or warning when both
touchdown=Trueandbackend="generalized"are configured.Suggested guard (e.g., in `__init__` or at the top of `unknown_constants`)
+ if self.config.touchdown and isinstance(self.eigensystem, GeneralizedEigensystem): + raise NotImplementedError( + "Touchdown is not supported with the generalized backend." + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/weac/core/system_model.py` around lines 253 - 278, When a generalized backend is used but touchdown parameters are set, add an explicit guard that rejects this unsupported combination: detect if isinstance(self.eigensystem, GeneralizedEigensystem) (or GeneralizedUnknownConstantsSolver selection) AND self.slab_touchdown is not None, and raise a clear exception (e.g., ValueError or RuntimeError) explaining that touchdown is only supported for the classical model; update the method that contains this logic (the unknown constants flow that selects GeneralizedUnknownConstantsSolver / UnknownConstantsSolver and calls solve_for_unknown_constants) to perform this check before calling the solver so the unsupported configuration fails fast instead of silently producing incorrect results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demo/generalized_backend_documentation.ipynb`:
- Around line 620-623: The variable name is misspelled: change the third
variable from x_weeklayer to x_weaklayer where
Analyzer().rasterize_solution(mode="cracked") is unpacked (currently x_slab,
z_slab, x_weeklayer) so it matches usages like x_weaklayer elsewhere in the
notebook; update the assignment to read x_slab, z_slab, x_weaklayer to maintain
consistency with the rasterize_solution return and subsequent references.
- Around line 258-266: The snippet passes an undefined field b into Config
causing pydantic ValidationError; update the call so Config(...) only uses its
defined fields (touchdown, backend, forced_touchdown_mode) and move the width
parameter into a ScenarioConfig instance (e.g., create ScenarioConfig(b=290) and
pass that where the scenario is constructed) or, if the intention was to make b
part of the top-level config, add b as a field on the Config model (rename or
type it consistently) and update Config's definition instead of passing b into
Config(); modify the symbol config_generalized accordingly so it no longer
includes an invalid b argument.
In `@src/weac/analysis/analyzer.py`:
- Around line 150-153: Replace brittle string comparisons of
self.sm.config.backend with a centralized boolean/property or constant: add a
property on the SystemModel (e.g., SystemModel.is_generalized) or a constant
(e.g., BACKEND_GENERALIZED) in system_model.py and return True when
config.backend equals the canonical constant; then update analyzer.py (the zs
allocation block using self.sm.config.backend=="generalized") to use
self.sm.is_generalized (or compare to BACKEND_GENERALIZED) instead, and change
all other occurrences (e.g., in system_model.py and any other modules) to use
the new property/constant so a typo won’t silently change behavior.
- Around line 691-709: The classic-branch call to self.sm.z currently uses qs=qs
if gi[idx] else 0 which is redundant and inconsistent with the other branch;
change the call in the classic branch (the z invocation used to compute Gdif via
self.sm.fq.Gi and self.sm.fq.Gii) to pass qs=qs unconditionally so SystemModel.z
handles zeroing when is_loaded=False, keeping behavior consistent with the
non-classic branch (and with the z calls used for z_ct/z_ub and subsequent
self.sm.fq.Gi/Gii/Giii calls).
In `@src/weac/components/scenario_config.py`:
- Around line 67-75: The Field for variable b in scenario_config.py uses an
unnecessary f-string for its description; update the b Field declaration (the
variable named b and its Field(...) call) to remove the leading f from the
description string so it is a plain string literal ("Out-of-plane width of the
model in [mm]") instead of f"...".
- Around line 100-109: The validator check_load_vector_shape currently reshapes
any array with 6 elements into (6,1) which can hide malformed 2D shapes; change
it to only accept inputs that are already 1-D length 6 or a column vector shape
(6,1): convert 1-D (6,) to (6,1) but raise ValueError for any other shapes
(e.g., (2,3), (3,2), or other 2-D shapes) and include the offending shape in the
error message; keep the dtype conversion to np.float64 and apply this logic in
the same field_validator for "load_vector_left" and "load_vector_right" inside
check_load_vector_shape.
In `@src/weac/core/generalized_unknown_constants_solver.py`:
- Around line 240-246: Replace the undocumented magic number 30 used in the
weak-layer assembly (zpi_weak_layer[30-local_end:]) with a named constant or
computed expression and update the nearby assembly lines in
generalized_unknown_constants_solver.py (the block manipulating Zh0_weak_layer,
Zhi_weak_layer, Zp0_weak_layer and zpi_weak_layer). For example, derive a
descriptive symbol like FULL_DOF_PLUS_BOUNDARY = full_dof_count + boundary_size
(or compute full_dof_count and boundary_size from existing variables) and use
zpi_weak_layer[FULL_DOF_PLUS_BOUNDARY - local_end :] (and similarly in any other
slice expressions) so the intent is clear and the magic number is removed; add a
brief inline comment explaining the constant if not obvious.
- Around line 452-484: The _setup_conditions_slab function (and similarly
_setup_conditions_weak_layer) can leave the local variable conditions unassigned
if pos doesn't match any handled branches; add a final else branch at the end of
each function to handle unexpected pos values by either initializing conditions
to a safe default (e.g., an appropriate-sized zero/empty numpy array) or,
preferably, raising a clear ValueError with context (include pos and function
name) so the caller fails fast; update references in _setup_conditions_slab and
_setup_conditions_weak_layer to ensure conditions is always defined before
logger.debug and return.
---
Outside diff comments:
In `@src/weac/core/system_model.py`:
- Around line 253-278: When a generalized backend is used but touchdown
parameters are set, add an explicit guard that rejects this unsupported
combination: detect if isinstance(self.eigensystem, GeneralizedEigensystem) (or
GeneralizedUnknownConstantsSolver selection) AND self.slab_touchdown is not
None, and raise a clear exception (e.g., ValueError or RuntimeError) explaining
that touchdown is only supported for the classical model; update the method that
contains this logic (the unknown constants flow that selects
GeneralizedUnknownConstantsSolver / UnknownConstantsSolver and calls
solve_for_unknown_constants) to perform this check before calling the solver so
the unsupported configuration fails fast instead of silently producing incorrect
results.
---
Duplicate comments:
In `@demo/generalized_backend_documentation.ipynb`:
- Around line 144-149: The WeakLayer model's E field is a float with ge=0 and
default 0.0, so passing E=None will trigger a pydantic ValidationError; update
the notebook example where WeakLayer(...) is instantiated (symbol: WeakLayer and
its parameter E) to either omit the E argument entirely or pass E=0.0 instead of
E=None so validation succeeds.
In `@src/weac/analysis/analyzer.py`:
- Line 19: Remove the unused top-level import "from weac.core import scenario"
in analyzer.py; the code always accesses scenario via the state machine
attribute (self.sm.scenario) so delete that import line and ensure no other
direct references to the module name "scenario" exist in Analyzer methods (e.g.,
any use in class Analyzer or functions that might expect the import).
In `@src/weac/core/generalized_field_quantities.py`:
- Around line 334-345: Add a missing theta parameter to dz_dx and dz_dxdx and
pass it into self.es.get_load_vector(theta=theta, phi=phi, qs=qs); then update
and propagate the new theta argument through the related methods du0_dxdx,
dpsi_dxdx, du0_dxdxdx, and dpsi_dxdxdx so each signature accepts theta and
forwards it when calling dz_dx/dz_dxdx or directly to self.es.get_load_vector,
ensuring all call sites are updated accordingly.
- Around line 11-13: Remove the unused imports G_MM_S2 and decompose_to_xyz from
the import block in generalized_field_quantities.py: keep the needed import
GeneralizedEigensystem from weac.core.generalized_eigensystem but delete
references to G_MM_S2 and decompose_to_xyz (they are not used in active code;
decompose_to_xyz only appears in commented-out code), and run linters to confirm
the Pylint E0611 warning is resolved; if those symbols are needed later,
reintroduce them only where actually used (e.g., in functions that call
decompose_to_xyz or use G_MM_S2).
- Around line 133-175: Update the copy-pasted docstrings to reflect the correct
amplitude type (constant vs linear) and physical direction for each symbol:
change theta_uc() docstring to "Constant amplitude of axial cosine shaped
displacements" (and adjust dtheta_uc_dx() to say "First derivative of the
constant amplitude..."), ensure theta_ul()/dtheta_ul_dx() retain "linear/first
derivative of linear" for axial, change theta_vc()/dtheta_vc_dx() to "Constant
amplitude of out-of-plane cosine shaped displacements"/"First derivative of the
constant amplitude of out-of-plane ...", theta_vl()/dtheta_vl_dx() to "Linear
amplitude of out-of-plane ..."/"First derivative of the linear amplitude of
out-of-plane ...", theta_wc()/dtheta_wc_dx() to "Constant amplitude of vertical
cosine shaped displacements"/"First derivative of the constant amplitude of
vertical ...", and theta_wl() to "Linear amplitude of vertical cosine shaped
displacements" (and dtheta_wl_dx() to the matching derivative description); use
the exact function names (theta_uc, dtheta_uc_dx, theta_ul, dtheta_ul_dx,
theta_vc, dtheta_vc_dx, theta_vl, dtheta_vl_dx, theta_wc, dtheta_wc_dx,
theta_wl, dtheta_wl_dx) to find and edit the docstrings.
- Around line 290-332: The functions Gi, Gii, and Giii accept a Z_back parameter
that is never used (triggering Pylint W0613) and large blocks of commented-out
reference code remain; either remove Z_back from the function signatures (and
update all callers) or rename it to a deliberately unused name (e.g., _Z_back:
np.ndarray) to silence the lint warning, update the docstrings to reflect the
change, and delete or move the ~40 lines of commented reference implementations
(or relocate them to external docs) to keep the file clean; apply this change to
the methods Gi, Gii, and Giii and any internal call sites that pass Z_back.
In `@src/weac/core/generalized_unknown_constants_solver.py`:
- Around line 17-24: Remove the unused imports and dead/commented code in
generalized_unknown_constants_solver.py: delete the unused G_MM_S2 and
get_skier_point_load imports and remove the commented-out "# from weac.constants
import G_MM_S2, LSKI_MM" line; verify whether decompose_to_xyz is referenced
anywhere in functions/methods in this module (if not used, remove its import as
well, otherwise keep it and ensure its usage is explicit), leaving only imports
actually referenced by GeneralizedEigensystem, GeneralizedFieldQuantities,
Scenario and SystemType.
- Around line 84-86: The code creates pi via pi = np.full(nS, "m") which yields
a <U1 array so pi[0] = "length" silently stores "l"; update the assembly of the
position vector (pi, pi[0], pi[-1]) to allocate a string-capable dtype (e.g.,
np.full(nS, "m", dtype=object) or an appropriately sized unicode dtype like
dtype='<U6'), or build pi as a Python list and convert to ndarray only if
needed, so that assigning "length" and "r" stores the full strings rather than
truncated characters.
In `@src/weac/core/system_model.py`:
- Around line 447-474: The z() code duplicates four nearly identical branches;
extract a small helper (e.g., _compute_zp_args or _call_zp) that builds and
calls self.eigensystem.zp with the correct arguments (include theta only when
config.backend == "generalized") and returns the zp term for a given xi (or
scalar x); then replace the list and scalar branches in z() to call
np.dot(self.eigensystem.zh(...), C) + helper(xi, ...) (and map over x when x is
list/array) so the only varying piece (presence/absence of theta) is handled
inside the helper while reusing self.eigensystem.zh, C, phi, has_foundation and
qs/is_loaded logic.
- Around line 333-335: The update_layers method currently recreates the Slab
with new_layers but loses the configured slab width b; modify update_layers
(method name: update_layers) to preserve the existing slab width by forwarding
the current value (self.slab.b) when constructing the new Slab (or by setting
the new Slab's b to self.slab.b after creation) so the previously configured b
from model_input.scenario_config is retained.
- Line 19: Remove the unused top-level import "from pydantic import config"
which shadows the local variable/parameter name "config" used across this module
(e.g., in the SystemModel class and its methods); delete that import line so the
local "config" parameter names remain unshadowed and ensure there are no other
references to pydantic.config in functions like __init__ or any methods before
committing.
Comment on lines
+258
to
+266
| "# Generalized backend configuration\n", | ||
| "\n", | ||
| "# Generalized backend without touchdown\n", | ||
| "config_generalized = Config(\n", | ||
| " backend=\"generalized\",\n", | ||
| " touchdown=False,\n", | ||
| " b=290\n", | ||
| ")" | ||
| ] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check Config class fields
echo "=== Searching for Config class definition ==="
rg -n "class Config" src/weac/components/config.py -A 20
echo ""
echo "=== Searching for Config fields ==="
rg -n "^\s+\w+.*:" src/weac/components/config.py | head -20
echo ""
echo "=== Searching for ScenarioConfig class and b field ==="
rg -n "class ScenarioConfig" src/weac/components/ -A 20
echo ""
echo "=== Checking for 'b:' field in scenario_config.py ==="
rg -n "^\s+b\s*:" src/weac/components/Repository: 2phi/weac
Length of output: 3287
🏁 Script executed:
#!/bin/bash
# Check the notebook file structure and the relevant code
echo "=== Checking notebook file type and size ==="
file demo/generalized_backend_documentation.ipynb
wc -l demo/generalized_backend_documentation.ipynb
echo ""
echo "=== Extract relevant section around lines 258-266 ==="
# Notebooks are JSON, so let's search for the Config construction
rg -n "Config\(" demo/generalized_backend_documentation.ipynb | head -20
echo ""
echo "=== Look for any Config with b parameter ==="
rg -n "b=290" demo/generalized_backend_documentation.ipynbRepository: 2phi/weac
Length of output: 632
🏁 Script executed:
#!/bin/bash
# Extract the exact content around the Config call
# Notebooks are JSON, so we need to be careful
python3 << 'EOF'
import json
with open('demo/generalized_backend_documentation.ipynb', 'r') as f:
nb = json.load(f)
# Find cells with Config references
for i, cell in enumerate(nb['cells']):
if cell['cell_type'] == 'code':
source = ''.join(cell['source'])
if 'config_generalized = Config' in source:
print(f"=== Cell {i} (Code cell) ===")
print(source)
print()
EOFRepository: 2phi/weac
Length of output: 247
Config has no b field — Config(..., b=290) will raise ValidationError.
The Config model only has touchdown, backend, and forced_touchdown_mode fields. Passing b=290 will cause a pydantic validation error because b is not defined on Config. The b field exists on ScenarioConfig instead. Either add b to Config or pass the width specification to ScenarioConfig instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@demo/generalized_backend_documentation.ipynb` around lines 258 - 266, The
snippet passes an undefined field b into Config causing pydantic
ValidationError; update the call so Config(...) only uses its defined fields
(touchdown, backend, forced_touchdown_mode) and move the width parameter into a
ScenarioConfig instance (e.g., create ScenarioConfig(b=290) and pass that where
the scenario is constructed) or, if the intention was to make b part of the
top-level config, add b as a field on the Config model (rename or type it
consistently) and update Config's definition instead of passing b into Config();
modify the symbol config_generalized accordingly so it no longer includes an
invalid b argument.
Comment on lines
+620
to
+623
| "# ===== ANALYZE =====\n", | ||
| "analyzer = Analyzer(system)\n", | ||
| "x_slab, z_slab, x_weeklayer = analyzer.rasterize_solution(mode=\"cracked\")\n", | ||
| "\n", |
Contributor
There was a problem hiding this comment.
Typo: x_weeklayer should be x_weaklayer.
Line 622 uses x_weeklayer but the variable returned by rasterize_solution is assigned as the third return value. The naming is inconsistent with x_weaklayer used in line 351 and other places in the notebook.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@demo/generalized_backend_documentation.ipynb` around lines 620 - 623, The
variable name is misspelled: change the third variable from x_weeklayer to
x_weaklayer where Analyzer().rasterize_solution(mode="cracked") is unpacked
(currently x_slab, z_slab, x_weeklayer) so it matches usages like x_weaklayer
elsewhere in the notebook; update the assignment to read x_slab, z_slab,
x_weaklayer to maintain consistency with the rasterize_solution return and
subsequent references.
Comment on lines
+150
to
+153
| if self.sm.config.backend=="generalized": | ||
| zs = np.full([24, xs.size], np.nan) | ||
| else: | ||
| zs = np.full([6, xs.size], np.nan) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Backend check should use a centralized helper or constant, not string comparison.
self.sm.config.backend == "generalized" is repeated here and in multiple other locations (system_model.py, analyzer.py). A typo in the string literal would silently fall through. Consider adding a helper property like self.sm.is_generalized or at minimum referencing a constant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/weac/analysis/analyzer.py` around lines 150 - 153, Replace brittle string
comparisons of self.sm.config.backend with a centralized boolean/property or
constant: add a property on the SystemModel (e.g., SystemModel.is_generalized)
or a constant (e.g., BACKEND_GENERALIZED) in system_model.py and return True
when config.backend equals the canonical constant; then update analyzer.py (the
zs allocation block using self.sm.config.backend=="generalized") to use
self.sm.is_generalized (or compare to BACKEND_GENERALIZED) instead, and change
all other occurrences (e.g., in system_model.py and any other modules) to use
the new property/constant so a typo won’t silently change behavior.
Comment on lines
+691
to
+709
| if self.sm.config.backend=="classic": | ||
| z = self.sm.z( | ||
| li[idx], C[:, [idx]], li[idx], phi=phi, theta=theta,has_foundation=ki[idx],is_loaded=gi[idx], qs=qs if gi[idx] else 0 | ||
| ) | ||
| Gdif[1:3, j] = np.concatenate( | ||
| (self.sm.fq.Gi(z, unit=unit), self.sm.fq.Gii(z, unit=unit)) | ||
| ) | ||
| else: | ||
| if ki[idx]: | ||
| z_ct = self.sm.z(li[idx], C[:,[idx]], li[idx], phi=phi, theta=theta, has_foundation=ki[idx], is_loaded=gi[idx], qs=qs) | ||
| z_ub = self.sm.z(li[idx], C[:,[idx]], li[idx], phi=phi, theta=theta, has_foundation=ki[idx], is_loaded=gi[idx], qs=qs) | ||
| else: | ||
| z_ct = self.sm.z(li[idx], C[:,[idx]], li[idx], phi=phi, theta=theta, has_foundation=ki[idx], is_loaded=gi[idx], qs=qs) | ||
| z_ub = self.sm.z(li[idx], C[:,[idx]], li[idx], phi=phi, theta=theta, has_foundation=ki[idx], is_loaded=gi[idx], qs=qs) | ||
| Gdif[1:,j] = np.concatenate( | ||
| (self.sm.fq.Gi(z_ct,z_ub, phi=phi, theta=theta, unit=unit), | ||
| self.sm.fq.Gii(z_ct,z_ub, phi=phi, theta=theta, unit=unit), | ||
| self.sm.fq.Giii(z_ct,z_ub, phi, theta, unit=unit),) | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant qs conditioning in classic branch.
Line 693 passes both is_loaded=gi[idx] and qs=qs if gi[idx] else 0. Since SystemModel.z() already zeros qs when is_loaded=False, the manual conditional is redundant and inconsistent with the generalized branch (lines 700-704) which passes qs=qs unconditionally.
Suggested consistency fix
if self.sm.config.backend=="classic":
z = self.sm.z(
- li[idx], C[:, [idx]], li[idx], phi=phi, theta=theta,has_foundation=ki[idx],is_loaded=gi[idx], qs=qs if gi[idx] else 0
+ li[idx], C[:, [idx]], li[idx], phi=phi, theta=theta, has_foundation=ki[idx], is_loaded=gi[idx], qs=qs
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/weac/analysis/analyzer.py` around lines 691 - 709, The classic-branch
call to self.sm.z currently uses qs=qs if gi[idx] else 0 which is redundant and
inconsistent with the other branch; change the call in the classic branch (the z
invocation used to compute Gdif via self.sm.fq.Gi and self.sm.fq.Gii) to pass
qs=qs unconditionally so SystemModel.z handles zeroing when is_loaded=False,
keeping behavior consistent with the non-classic branch (and with the z calls
used for z_ct/z_ub and subsequent self.sm.fq.Gi/Gii/Giii calls).
Comment on lines
+67
to
+75
| theta: float = Field( | ||
| default=0.0, | ||
| ge=-90.0, | ||
| le=90.0, | ||
| description="Rotation angle in degrees (counterclockwise positive)", | ||
| ) | ||
| b: float = Field( | ||
| default = 300, ge=1, description= f"Out-of-plane width of the model in [mm]" | ||
| ) |
Contributor
There was a problem hiding this comment.
Unnecessary f-string prefix on b field description.
Line 74 uses f"Out-of-plane width of the model in [mm]" but contains no interpolation. Drop the f prefix.
Proposed fix
b: float = Field(
- default = 300, ge=1, description= f"Out-of-plane width of the model in [mm]"
+ default=300, ge=1, description="Out-of-plane width of the model in [mm]"
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/weac/components/scenario_config.py` around lines 67 - 75, The Field for
variable b in scenario_config.py uses an unnecessary f-string for its
description; update the b Field declaration (the variable named b and its
Field(...) call) to remove the leading f from the description string so it is a
plain string literal ("Out-of-plane width of the model in [mm]") instead of
f"...".
Comment on lines
+100
to
+109
| @field_validator("load_vector_left", "load_vector_right", mode="after") | ||
| def check_load_vector_shape(cls, value: Any) -> Any: | ||
| # Convert to numpy array if needed | ||
| arr = np.asarray(value, dtype=np.float64) | ||
| # Ensure correct shape (6, 1) | ||
| if arr.shape != (6, 1): | ||
| # Try to reshape if possible | ||
| arr = arr.reshape(-1, 1) | ||
| if arr.shape[0] != 6: | ||
| raise ValueError(f"load vectors must have shape (6, 1), got {arr.shape}") |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Validator silently reshapes arbitrarily-shaped arrays into (6, 1).
If someone passes, e.g., a (2, 3) or (3, 2) array that happens to have 6 total elements, reshape(-1, 1) will silently succeed and produce a (6, 1) vector. This could mask input errors. Consider rejecting inputs that aren't already 1-D or column vectors.
Suggested tightening
`@field_validator`("load_vector_left", "load_vector_right", mode="after")
def check_load_vector_shape(cls, value: Any) -> Any:
arr = np.asarray(value, dtype=np.float64)
if arr.shape != (6, 1):
- arr = arr.reshape(-1, 1)
- if arr.shape[0] != 6:
+ if arr.ndim > 2 or arr.size != 6:
raise ValueError(f"load vectors must have shape (6, 1), got {arr.shape}")
+ arr = arr.reshape(6, 1)
return arr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/weac/components/scenario_config.py` around lines 100 - 109, The validator
check_load_vector_shape currently reshapes any array with 6 elements into (6,1)
which can hide malformed 2D shapes; change it to only accept inputs that are
already 1-D length 6 or a column vector shape (6,1): convert 1-D (6,) to (6,1)
but raise ValueError for any other shapes (e.g., (2,3), (3,2), or other 2-D
shapes) and include the offending shape in the error message; keep the dtype
conversion to np.float64 and apply this logic in the same field_validator for
"load_vector_left" and "load_vector_right" inside check_load_vector_shape.
Comment on lines
+240
to
+246
| Zh0_weak_layer[ | ||
| global_start_weak_layer + local_start : global_start_weak_layer + local_end, | ||
| global_start_slab : global_start_slab + nDOF_segment ] = np.concatenate([ | ||
| Zhi_weak_layer[6 + local_start : 12, :], Zhi_weak_layer[6-local_end:, :]], | ||
| axis = 0) | ||
| Zp0_weak_layer[ | ||
| global_start_weak_layer + local_start : global_start_weak_layer + local_end] += np.concatenate([zpi_weak_layer[6 + local_start : 12], zpi_weak_layer[30-local_end:]], axis = 0) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Magic number 30 in weak-layer assembly is undocumented.
Line 246 uses zpi_weak_layer[30-local_end:] where 30 appears to be 24 + 6 (full DOF count plus boundary size). This should be expressed symbolically or documented with a comment to prevent maintenance errors.
Suggested clarification
+ # 30 = nDOF_supported + 6 boundary conditions
Zp0_weak_layer[
global_start_weak_layer + local_start : global_start_weak_layer + local_end] += np.concatenate([zpi_weak_layer[6 + local_start : 12], zpi_weak_layer[30-local_end:]], axis = 0)Or better yet, introduce a named constant.
📝 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.
Suggested change
| Zh0_weak_layer[ | |
| global_start_weak_layer + local_start : global_start_weak_layer + local_end, | |
| global_start_slab : global_start_slab + nDOF_segment ] = np.concatenate([ | |
| Zhi_weak_layer[6 + local_start : 12, :], Zhi_weak_layer[6-local_end:, :]], | |
| axis = 0) | |
| Zp0_weak_layer[ | |
| global_start_weak_layer + local_start : global_start_weak_layer + local_end] += np.concatenate([zpi_weak_layer[6 + local_start : 12], zpi_weak_layer[30-local_end:]], axis = 0) | |
| Zh0_weak_layer[ | |
| global_start_weak_layer + local_start : global_start_weak_layer + local_end, | |
| global_start_slab : global_start_slab + nDOF_segment ] = np.concatenate([ | |
| Zhi_weak_layer[6 + local_start : 12, :], Zhi_weak_layer[6-local_end:, :]], | |
| axis = 0) | |
| # 30 = nDOF_supported + 6 boundary conditions | |
| Zp0_weak_layer[ | |
| global_start_weak_layer + local_start : global_start_weak_layer + local_end] += np.concatenate([zpi_weak_layer[6 + local_start : 12], zpi_weak_layer[30-local_end:]], axis = 0) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/weac/core/generalized_unknown_constants_solver.py` around lines 240 -
246, Replace the undocumented magic number 30 used in the weak-layer assembly
(zpi_weak_layer[30-local_end:]) with a named constant or computed expression and
update the nearby assembly lines in generalized_unknown_constants_solver.py (the
block manipulating Zh0_weak_layer, Zhi_weak_layer, Zp0_weak_layer and
zpi_weak_layer). For example, derive a descriptive symbol like
FULL_DOF_PLUS_BOUNDARY = full_dof_count + boundary_size (or compute
full_dof_count and boundary_size from existing variables) and use
zpi_weak_layer[FULL_DOF_PLUS_BOUNDARY - local_end :] (and similarly in any other
slice expressions) so the intent is clear and the magic number is removed; add a
brief inline comment explaining the constant if not obvious.
Comment on lines
+452
to
+484
| elif pos in ("r", "right"): | ||
| bcs = cls._boundary_conditions_slab( | ||
| zr, | ||
| eigensystem, | ||
| has_foundation, | ||
| pos, | ||
| system_type, | ||
| ) # Right boundary condition | ||
| #has_foundation=False | ||
| conditions = np.array( | ||
| [ | ||
| -fq.u(zl, h0=0, b0=0), # -ui(xi = 0) | ||
| -fq.v(zl, h0=0), # -vi(xi = 0) | ||
| -fq.w(zl, b0=0), # -wi(xi = 0) | ||
| -fq.psix(zl), # -psixi(xi = 0) | ||
| -fq.psiy(zl), # -psiyi(xi = 0) | ||
| -fq.psiz(zl), # -psizi(xi = 0) | ||
| -fq.Nx(zl, has_foundation), # -Nxi(xi = 0) | ||
| -fq.Vy(zl, has_foundation), # -Vyi(xi = 0) | ||
| -fq.Vz(zl, has_foundation), # -Vzi(xi = 0) | ||
| -fq.Mx(zl, has_foundation), # -Mxi(xi = 0) | ||
| -fq.My(zl, has_foundation), # -Myi(xi = 0) | ||
| -fq.Mz(zl, has_foundation), # -Mzi(xi = 0) | ||
| bcs[0], | ||
| bcs[1], | ||
| bcs[2], | ||
| bcs[3], | ||
| bcs[4], | ||
| bcs[5], | ||
| ] | ||
| ) | ||
| logger.debug("Boundary Conditions at pos %s: %s", pos, conditions.shape) # pylint: disable=E0606 | ||
| return conditions |
Contributor
There was a problem hiding this comment.
UnboundLocalError if pos doesn't match any branch in _setup_conditions_slab.
If pos is not in ("l", "left", "m", "mid", "r", "right"), conditions is never assigned and line 483 crashes. The same issue exists in _setup_conditions_weak_layer (line 605). Add an else guard to both methods.
Proposed fix (apply to both methods)
elif pos in ("r", "right"):
...
+ else:
+ raise ValueError(f"Unknown position: {pos!r}")
logger.debug("Boundary Conditions at pos %s: %s", pos, conditions.shape)📝 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.
Suggested change
| elif pos in ("r", "right"): | |
| bcs = cls._boundary_conditions_slab( | |
| zr, | |
| eigensystem, | |
| has_foundation, | |
| pos, | |
| system_type, | |
| ) # Right boundary condition | |
| #has_foundation=False | |
| conditions = np.array( | |
| [ | |
| -fq.u(zl, h0=0, b0=0), # -ui(xi = 0) | |
| -fq.v(zl, h0=0), # -vi(xi = 0) | |
| -fq.w(zl, b0=0), # -wi(xi = 0) | |
| -fq.psix(zl), # -psixi(xi = 0) | |
| -fq.psiy(zl), # -psiyi(xi = 0) | |
| -fq.psiz(zl), # -psizi(xi = 0) | |
| -fq.Nx(zl, has_foundation), # -Nxi(xi = 0) | |
| -fq.Vy(zl, has_foundation), # -Vyi(xi = 0) | |
| -fq.Vz(zl, has_foundation), # -Vzi(xi = 0) | |
| -fq.Mx(zl, has_foundation), # -Mxi(xi = 0) | |
| -fq.My(zl, has_foundation), # -Myi(xi = 0) | |
| -fq.Mz(zl, has_foundation), # -Mzi(xi = 0) | |
| bcs[0], | |
| bcs[1], | |
| bcs[2], | |
| bcs[3], | |
| bcs[4], | |
| bcs[5], | |
| ] | |
| ) | |
| logger.debug("Boundary Conditions at pos %s: %s", pos, conditions.shape) # pylint: disable=E0606 | |
| return conditions | |
| elif pos in ("r", "right"): | |
| bcs = cls._boundary_conditions_slab( | |
| zr, | |
| eigensystem, | |
| has_foundation, | |
| pos, | |
| system_type, | |
| ) # Right boundary condition | |
| `#has_foundation`=False | |
| conditions = np.array( | |
| [ | |
| -fq.u(zl, h0=0, b0=0), # -ui(xi = 0) | |
| -fq.v(zl, h0=0), # -vi(xi = 0) | |
| -fq.w(zl, b0=0), # -wi(xi = 0) | |
| -fq.psix(zl), # -psixi(xi = 0) | |
| -fq.psiy(zl), # -psiyi(xi = 0) | |
| -fq.psiz(zl), # -psizi(xi = 0) | |
| -fq.Nx(zl, has_foundation), # -Nxi(xi = 0) | |
| -fq.Vy(zl, has_foundation), # -Vyi(xi = 0) | |
| -fq.Vz(zl, has_foundation), # -Vzi(xi = 0) | |
| -fq.Mx(zl, has_foundation), # -Mxi(xi = 0) | |
| -fq.My(zl, has_foundation), # -Myi(xi = 0) | |
| -fq.Mz(zl, has_foundation), # -Mzi(xi = 0) | |
| bcs[0], | |
| bcs[1], | |
| bcs[2], | |
| bcs[3], | |
| bcs[4], | |
| bcs[5], | |
| ] | |
| ) | |
| else: | |
| raise ValueError(f"Unknown position: {pos!r}") | |
| logger.debug("Boundary Conditions at pos %s: %s", pos, conditions.shape) # pylint: disable=E0606 | |
| return conditions |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/weac/core/generalized_unknown_constants_solver.py` around lines 452 -
484, The _setup_conditions_slab function (and similarly
_setup_conditions_weak_layer) can leave the local variable conditions unassigned
if pos doesn't match any handled branches; add a final else branch at the end of
each function to handle unexpected pos values by either initializing conditions
to a safe default (e.g., an appropriate-sized zero/empty numpy array) or,
preferably, raising a clear ValueError with context (include pos and function
name) so the caller fails fast; update references in _setup_conditions_slab and
_setup_conditions_weak_layer to ensure conditions is always defined before
logger.debug and return.
…layer calculations
…o relevant functions during testing
…ructor for clarity
- Removed unused imports in `generalized_field_quantities.py`. - Cleaned up whitespace and formatting inconsistencies in `generalized_field_quantities.py`. - Updated comments for clarity in `scenario.py`. - Standardized formatting in `misc.py` for function parameters.
pillowbeast
reviewed
Feb 20, 2026
Collaborator
There was a problem hiding this comment.
Use x/y/z instead of tan/norm as well?
pillowbeast
reviewed
Feb 20, 2026
| h : float | ||
| Height/Thickness of the layer [mm]. | ||
| f : float | ||
| Resultant force of the layer [N/mm] |
Collaborator
There was a problem hiding this comment.
resultant force or weight density?
pillowbeast
reviewed
Feb 20, 2026
| h: float = Field(default=20, gt=0, description="Height/Thickness of the slab [mm]") | ||
| f: float | None = Field( | ||
| default=None, description="Weight density of the weak layer [N/mm^3]" | ||
| ) |
Collaborator
There was a problem hiding this comment.
resultant force or weight density?
pillowbeast
reviewed
Feb 20, 2026
|
|
||
| 1. **Eigensystem**: Computes eigenvalues/eigenvectors for the beam-foundation system | ||
| 2. **Slab Touchdown** (if enabled): Calculates touchdown behavior and updates segment lengths | ||
| 2. **Slab Touchdown** (if enabled): Calculates touchdown behavior and updates segment lengths (only available for the classical model) |
Collaborator
There was a problem hiding this comment.
@coderabbitai check if we catch that slab touchdown and general backend cannot be activated at the same time
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for validation logic that checks both touchdown and backend together
rg -n -C5 'touchdown.*backend|backend.*touchdown' --type=py
# Search for validation or error raising related to touchdown in Config
rg -n -C5 'class Config' src/weac/components/config.py
# Search for any validation in SystemModel related to touchdown and backend
rg -n -C5 'def __init__.*ModelInput|def slab_touchdown|if.*touchdown.*backend|if.*backend.*touchdown' src/weac/core/system_model.py
# Look for any ValueError or RuntimeError related to touchdown
rg -n -C3 'raise.*touchdown|raise.*backend' --type=py
Length of output: 1275
🏁 Script executed:
#!/bin/bash
# Check SlabTouchdown class for any backend checks
rg -n -A10 'class SlabTouchdown' src/weac/core/slab_touchdown.py | head -30
# Check if Config has any model_validator that could check both fields
rg -n 'model_validator|field_validator' src/weac/components/config.py
# Check if there's any runtime check when slab_touchdown is accessed
ast-grep --pattern $'if $_.config.touchdown:
$$$
if $_.is_generalized:
$$$'
# Look for any comments or TODOs about this incompatibility
rg -n -i 'todo.*touchdown.*backend|todo.*backend.*touchdown|fixme.*touchdown|fixme.*backend' --type=py
Length of output: 649
@pillowbeast Good catch! The codebase does not currently validate that slab touchdown and the generalized backend are mutually exclusive.
Current state:
- The
Configclass accepts bothtouchdown=Trueandbackend="generalized"without validation - The docstring at Line 76 documents that touchdown is "(only available for the classical model)"
- However, no
model_validatorinConfigor runtime check inSystemModel.__init__enforces this constraint
Risk:
Users can create a configuration with both features enabled, which could lead to undefined behavior since the touchdown logic (SlabTouchdown class) was designed for the classical 6-DOF system, not the generalized 24-DOF system.
Recommended fix:
Add a Pydantic model_validator to the Config class:
from pydantic import model_validator
class Config(BaseModel):
# ... existing fields ...
`@model_validator`(mode='after')
def validate_touchdown_backend_compatibility(self) -> 'Config':
if self.touchdown and self.backend == "generalized":
raise ValueError(
"Slab touchdown is only available for the classical backend. "
"Set backend='classic' or disable touchdown."
)
return selfThis would catch the incompatibility at configuration time and provide a clear error message to users.
🧬 Code Graph Analysis Results
-
File: src/weac/components/config.py, lines 20-50
Snippet:class Config(BaseModel): """ Configuration for the WEAC simulation. Attributes ---------- touchdown : bool Whether slab touchdown on the collapsed weak layer is considered. backend : Literal["classic", "generalized"] Selects which eigensystem implementation to use under the hood. forced_touchdown_mode : TouchdownMode | None If set, forces the touchdown mode instead of calculating it from l_AB/l_BC. This avoids floating-point precision issues when scenario parameters change. """ touchdown: bool = Field( default=False, description="Whether to include slab touchdown in the analysis" ) backend: Literal["classic", "generalized"] = Field( default="classic", description=( "Eigensystem backend: 'classic' uses the current WEAC solver; " "'generalized' routes to an advanced adapter (initially identical behavior)." ), ) forced_touchdown_mode: TouchdownMode | None = Field( default=None, description="Force a specific touchdown mode instead of auto-calculating", )Relevance: Defines the flags for enabling slab touchdown and toggling between the classic and generalized backends, which is central to the user concern about not allowing touchdown and generalized backend to be active simultaneously.
-
File: src/weac/core/system_model.py, relevant concept: is_generalized, slab_touchdown, and unknown_constants flow
Summary: is_generalized flag and backend selection- Purpose
- Determine which backend to use for solving the eigensystem (classic vs generalized).
- Parameters
- None beyond internal config (self.config.backend).
- Returns
- Boolean indicating whether the generalized backend is selected.
- Behavior notes
- Used to decide which FieldQuantities class to instantiate (GeneralizedFieldQuantities vs FieldQuantities).
- Important details
- Affects how unknown constants are solved and how the field quantities are computed.
Summary: Slab touchdown (slab_touchdown) workflow
- Purpose
- When config.touchdown is enabled, compute touchdown behavior and adjust the scenario accordingly.
- Parameters
- self.scenario, self.eigensystem, self.config.forced_touchdown_mode
- Returns
- SlabTouchdown instance if touchdown is performed, else None.
- Side effects
- Updates self.scenario to use touchdown_length as the segment length (for PST/v PST variants) and logs the updated segment lengths.
- Error handling
- If touchdown is disabled, returns None (no exception path shown here).
- Important details
- Touchdown can redefine the scenario segments to reflect touchdown distance, aligning with the original WEAC behavior.
Summary: Unknown constants solution selection
- Purpose
- Solve the constants C for the beam-on-elastic-foundation problem, using either UnknownConstantsSolver or GeneralizedUnknownConstantsSolver depending on the eigensystem backend.
- Parameters
- scenario, eigensystem, system_type, and optional touchdown_distance, touchdown_mode, collapsed_weak_layer_kR
- Returns
- A numpy ndarray with the solution constants (shape depends on number of segments and backend; e.g., (N, 6) for classic, or (N, 24) for generalized in some code paths).
- Error handling
- Logs progress and switches solver based on isinstance(self.eigensystem, GeneralizedEigensystem).
- Important details
- If slab_touchdown is present, passes touchdown_distance and touchdown_mode to the solver to incorporate touchdown constraints into the unknown constants.
- Cached via
@cached_propertyin typical usage, enabling lazy evaluation.
Relevance to user comment: These elements govern the interaction between slab touchdown logic and the generalized backend. The user’s note about “catching that slab touchdown and general backend cannot be activated at the same time” hinges on:
- The backend toggle (classic vs generalized)
- The touchdown pathway (slab_touchdown) and its effect on the scenario
- The unknown constants solver selecting the correct path depending on the backend
- Purpose
-
File: src/weac/core/unknown_constants_solver.py, lines 1-60 (illustrative)
Snippet:class UnknownConstantsSolver: """ This class solves the unknown constants for the WEAC simulation. """ `@classmethod` def solve_for_unknown_constants( cls, scenario: Scenario, eigensystem: Eigensystem, system_type: SystemType, touchdown_distance: float | None = None, touchdown_mode: Literal["A_free_hanging", "B_point_contact", "C_in_contact"] | None = None, collapsed_weak_layer_kR: float | None = None, ) -> np.ndarray: """ Compute free constants *C* for system. - Assembles linear system reflecting transmission/boundary conditions between segments - Uses zh(x) and zp(x) from the provided eigensystem - Applies touchdown parameters (distance and mode) if provided - Solves for constants C via Zh0 * C = rhs - Zp0 - Returns C as a 2D array with one column per segment (6 constants per segment) - Handles basic LinAlgError diagnostics in failure cases """ ...Relevance: This function embodies how the solution constants are computed with or without touchdown and how the system type (including PST/vpst or skier-type boundary conditions) affects the system assembly. It directly supports the user’s concern about ensuring that enabling touchdown and enabling generalized backend cannot both be active in a conflicting way, since this flow determines which solver path is used.
-
File: src/weac/core/eigensystem.py, relevant to backend choice
Summary: Eigensystem base behavior- Purpose
- Compute eigenvalues/eigenvectors for the 6x6 (classic) system, or the generalized 24x24 system in the generalized backend.
- Return values
- For classic: eb C, eb R, evC, evR, sR, sC for 6x6 formulation
- For generalized: analogous structures with expanded dimensionality
- Important details
- The choice between Eigensystem (classic) and GeneralizedEigensystem (generalized) drives downstream calculations (slab touches down, unknown constants solver path, FieldQuantities type, etc.)
- Interaction
- SystemModel.eigensystem performs the heavy lifting and is used to decide:
- which FieldQuantities class to instantiate
- which UnknownConstantsSolver to call
- whether touchdown logic should modify the scenario before solving constants
- SystemModel.eigensystem performs the heavy lifting and is used to decide:
Relevance: This underpins the runtime decision of backend type and its compatibility with touchdown workflow. The user is asking to ensure these two features do not get used together in a conflicting way; understanding how the two backends are selected and how touchdown modifies the scenario helps reviewers verify the guardrails.
- Purpose
Notes
- The concrete code blocks above illustrate the integration points between:
- touchdown enablement (config.touchdown)
- backend selection (config.backend and SystemModel.is_generalized / eigensystem type checks)
- unknown constants solving (UnknownConstantsSolver vs GeneralizedUnknownConstantsSolver)
- If you want explicit enforcement (e.g., preventing enabling touchdown when backend is generalized), you would add validation logic near configuration initialization or in the SystemModel init to raise an error if self.config.touchdown and self.is_generalized are both true. The current structure indicates these conditions are allowed to interact through the solver flow, with the generalized path potentially supporting more DOF in constants (e.g., 24 vs 6).
Merged
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
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.
Implementation of the variational model for out-of-plane bending as an alternative backend model.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes