Refactor of Weac #8
Conversation
|
Warning Rate limit exceeded@zacczakk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
WalkthroughReplace legacy procedural/mixin code with a new object-oriented core (Slab, Scenario, Eigensystem, SlabTouchdown, SystemModel, UnknownConstantsSolver, FieldQuantities), add Pydantic component models and analysis layer (Analyzer, CriteriaEvaluator, Plotter), introduce constants/utilities, CI workflows, many tests/fixtures, demo scripts, a data submodule, and remove numerous legacy modules and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Components as Components (ModelInput, Config)
participant System as SystemModel
participant Core as Eigensystem/Scenario/Solver
participant Analysis as Analyzer/CriteriaEvaluator
participant Plot as Plotter
User->>Components: create ModelInput + Config
User->>System: instantiate SystemModel(model_input, config)
System->>Core: build Slab and Scenario
System->>Core: compute cached Eigensystem
System->>Core: UnknownConstantsSolver.solve_for_unknown_constants(...)
System-->>User: provide field quantities (fq) and z(...)
User->>Analysis: Analyzer(SystemModel) / CriteriaEvaluator(...)
Analysis->>System: request rasterize_solution / find_minimum_force / find_crack_length
Analysis-->>User: raster, stresses, ERR, CoupledCriterionResult
User->>Plot: Plotter.plot_*(Analyzer/SystemModel)
Plot-->>User: saved figure files
sequenceDiagram
participant System as SystemModel
participant TD as SlabTouchdown
participant Solver as UnknownConstantsSolver
System->>TD: instantiate if config.touchdown True
TD-->>System: touchdown_mode, touchdown_distance, collapsed_kR
System->>Solver: solve_for_unknown_constants(..., touchdown params)
Solver-->>System: unknown constants C
Estimated code review effort🎯 5 (Critical) | ⏱️ ~180 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Duplicate Methods Cause Redundant Computation
The methods _create_collapsed_eigensystem and _create_collapsed_system are duplicates with identical logic. Both create a collapsed weak layer with modified stiffness and its associated eigensystem, though _create_collapsed_eigensystem takes an unused qs parameter. This leads to redundant, expensive computation as both are called, with the second call overwriting the first's results, and introduces maintenance overhead.
weac_2/core/slab_touchdown.py#L203-L239
weac/weac_2/core/slab_touchdown.py
Lines 203 to 239 in 9b40fef
BugBot free trial expires on July 22, 2025
You have used $0.00 of your $50.00 spend limit so far. Manage your spend limit in the Cursor dashboard.
Was this report helpful? Give feedback by reacting with 👍 or 👎
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🔭 Outside diff range comments (1)
.github/workflows/pylint.yml (1)
30-68: Upload pylint-report.txt as an artifact for reviewersYou already generate a useful report; persist it for PR reviewers and history.
- name: Run pylint analysis # Using repository pylint config (pyproject.toml) with comprehensive settings for scientific code run: | exit_code=0 TARGET_DIRS="" for d in weac weac_2 tests tests_2; do if [ -d "$d" ]; then TARGET_DIRS="$TARGET_DIRS $d"; fi done if [ -z "$TARGET_DIRS" ]; then echo "No target directories found for pylint (checked: weac, weac_2, tests, tests_2)." # Exit early without failing CI to avoid false negatives on empty repos. exit 0 fi python -m pylint --rcfile=pyproject.toml --output-format=parseable --output=pylint-report.txt $TARGET_DIRS || exit_code=$? echo "Pylint finished with exit code $exit_code." echo echo "Pylint exit code meaning:" if [ $exit_code -eq 0 ]; then echo "-> No issues found"; fi if [ $((exit_code & 1)) -ne 0 ]; then echo "-> Fatal message issued"; fi if [ $((exit_code & 2)) -ne 0 ]; then echo "-> Error message issued"; fi if [ $((exit_code & 4)) -ne 0 ]; then echo "-> Warning message issued"; fi if [ $((exit_code & 8)) -ne 0 ]; then echo "-> Refactor message issued"; fi if [ $((exit_code & 16)) -ne 0 ]; then echo "-> Convention message issued"; fi if [ $((exit_code & 32)) -ne 0 ]; then echo "-> Usage error"; fi echo echo 'Error type counts:' - grep -oP '[A-Z]\d+\([a-z\-]+\)' pylint-report.txt | sort | uniq -c | sort -nr + grep -oE '[A-Z][0-9]+\([a-z-]+\)' pylint-report.txt | sort | uniq -c | sort -nr echo echo 'Errors per file:' - grep -oP '^[\w\-\/]+\.py' pylint-report.txt | sort | uniq -c | sort -nr + grep -oE '^[[:alnum:]_.\/-]+\.py' pylint-report.txt | sort | uniq -c | sort -nr echo echo 'Total errors:' - grep -oP '^[\w\-\/]+\.py' pylint-report.txt | wc -l + grep -oE '^[[:alnum:]_.\/-]+\.py' pylint-report.txt | wc -l echo grep 'Your code' pylint-report.txt || true # Fail on fatal, error, and usage error. # These are severe and should block PRs. # Warnings (4), refactors (8), and conventions (16) will not cause a failure. fail_on_codes=$((1 | 2 | 32)) if [ $((exit_code & fail_on_codes)) -ne 0 ]; then echo "Failing CI due to fatal/error/usage messages from pylint." exit 1 else echo "Pylint check passed. No fatal/error/usage messages." fi + + - name: Upload pylint report + if: always() + uses: actions/upload-artifact@v4 + with: + name: pylint-report + path: pylint-report.txt + if-no-files-found: ignore + retention-days: 7
♻️ Duplicate comments (27)
.github/workflows/pylint.yml (2)
3-10: Good fix: quoted “on” key resolves yamllint truthy warningThe top-level on key is now quoted. This aligns with yamllint rules and prior feedback.
47-54: Replace grep -P with portable -E patternsGNU grep on ubuntu supports -P, but -P can be disabled elsewhere. Using -E is more portable with equivalent patterns.
- grep -oP '[A-Z]\d+\([a-z\-]+\)' pylint-report.txt | sort | uniq -c | sort -nr + grep -oE '[A-Z][0-9]+\([a-z-]+\)' pylint-report.txt | sort | uniq -c | sort -nr @@ - grep -oP '^[\w\-\/]+\.py' pylint-report.txt | sort | uniq -c | sort -nr + grep -oE '^[[:alnum:]_.\/-]+\.py' pylint-report.txt | sort | uniq -c | sort -nr @@ - grep -oP '^[\w\-\/]+\.py' pylint-report.txt | wc -l + grep -oE '^[[:alnum:]_.\/-]+\.py' pylint-report.txt | wc -ltests/utils/test_misc.py (1)
109-112: Clarify tangential comment for negative angles (match implementation/sign convention)The code computes
expected_tan = -f * sin(phi). For negative φ, this is positive; the comment currently suggests “magnitude f*sin(phi)” which can confuse since sin(phi) is negative. Tighten the wording.- # Normal component should still be positive and equal to f*cos(phi) - # Tangential should be positive (upslope for negative angle) with magnitude f*sin(phi) + # Normal component equals f*cos(phi) (cos is even ⇒ cos(-φ) = cos(φ)) + # Tangential equals -f*sin(phi); for negative φ this becomes positive with magnitude f*sin(|φ|)README.md (5)
182-187: Fix typo in header: “Exampel” → “Example”Minor but user-facing.
-# Exampel 2: PST +# Example 2: PST
216-217: Grammar: “cached property” (not “cached_properties”)Tighten the sentence and avoid the underscore.
-Unknown constants are cached_properties; calling `skier_system.unknown_constants` solves the system of linear equations and extracts the constants. +Unknown constants are a cached property; calling `skier_system.unknown_constants` solves the system of linear equations and extracts the constants.
261-271: Replace undefined placeholders (top/mid/bot) with concrete values and fix “weaklayer” typoAs written,
top,mid,botare undefined; give runnable values or explain them. Also fix the comment wording.-# Compute stresses in kPa in the weaklayer +# Compute stresses in kPa in the weak layer tau = skier_system.fq.tau(Z=z_skier, unit='kPa') sig = skier_system.fq.sig(Z=z_skier, unit='kPa') w = skier_system.fq.w(Z=z_skier, unit='um') -# Example evaluation vertical displacement at top/mid/bottom of the slab -u_top = skier_system.fq.u(Z=z_skier, h0=top, unit='um') -u_mid = skier_system.fq.u(Z=z_skier, h0=mid, unit='um') -u_bot = skier_system.fq.u(Z=z_skier, h0=bot, unit='um') +# Example: vertical displacement at top/mid/bottom of the weak layer (mm from top surface) +u_top = skier_system.fq.u(Z=z_skier, h0=0.0, unit='um') +u_mid = skier_system.fq.u(Z=z_skier, h0=10.0, unit='um') +u_bot = skier_system.fq.u(Z=z_skier, h0=20.0, unit='um') psi = skier_system.fq.psi(Z=z_skier, unit='deg')
360-366: Indent fenced block correctly, remove trailing spaces, and surround with blank linesKeeps ordered list numbering intact and satisfies markdownlint MD031/MD029.
-2. Initialize submodules - - ```bash - git submodule update --init --recursive - ``` +2. Initialize submodules + + ```bash + git submodule update --init --recursive + ``` +
200-206: Use the defined layers variable; custom_layers is undefinedThe snippet above defines
layers. Usingcustom_layerswill raise a NameError if readers copy/paste.model_input = ModelInput( weak_layer=weak_layer, scenario_config=skier_config, - layers=custom_layers, + layers=layers, segments=skier_segments, )tests/run_tests.py (1)
29-37: Discover tests in both tests and tests_2 to match the refactorThe PR mentions tests migrating to tests_2. Discover both directories when present.
- # Discover all tests in the tests directory (recursive by default) - test_suite = unittest.defaultTestLoader.discover( - test_dir, pattern="test_*.py", top_level_dir=parent_dir - ) - - # Count and display discovered tests - test_count = test_suite.countTestCases() - print(f"Found {test_count} test cases") + # Discover in tests and optionally tests_2 (recursive by default) + test_dirs = [test_dir] + alt_dir = os.path.join(parent_dir, "tests_2") + if os.path.isdir(alt_dir): + test_dirs.append(alt_dir) + + suites = [ + unittest.defaultTestLoader.discover(d, pattern="test_*.py", top_level_dir=parent_dir) + for d in test_dirs + ] + test_suite = unittest.TestSuite(suites) + + # Count and display discovered tests + test_count = test_suite.countTestCases() + print(f"Found {test_count} test cases across: {', '.join(test_dirs)}").github/workflows/tests.yml (3)
4-11: LGTM: quoting “on” and corrected indentation fix YAML linting issuesTrigger configuration and indentation look correct.
20-24: Speed up installs by enabling pip cachingCaching pip downloads noticeably reduces CI time.
- name: Set up Python 3.12 uses: actions/setup-python@v5 with: python-version: "3.12" + cache: "pip"
30-34: Optionally install dev extras if tests rely on themIf test tooling or linters are in the dev extra, install with extras.
- name: Install dependencies run: | python -m pip install --upgrade pip - python -m pip install -e . + python -m pip install -e ".[dev]"weac/components/scenario_config.py (4)
7-7: Import ConfigDict to support field aliases (for backward compatibility)We’ll use an alias for legacy configs (
system) and need ConfigDict.-from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, ConfigDict
41-52: Add alias for legacysystemkey and clarify allowed values in descriptionKeeps old configs working and avoids doc/field mismatches.
- system_type: SystemType = Field( - default="skiers", - description="Type of system, '-pst', 'pst-', ....; \n" - "skier: single skier in-between two segments, \n" - "skiers: multiple skiers spread over the slope, \n" - "pst-: positive PST: down-slope + slab-normal cuts, \n" - "-pst: negative PST: up-slope + slab-normal cuts, \n" - "rot: rotation: rotation of the slab, \n" - "trans: translation: translation of the slab, \n" - "vpst-: positive VPST: down-slope + vertical cuts, \n" - "-vpst: negative VPST: up-slope + vertical cuts, \n", - ) + # Accept legacy key `system` (Pydantic v2) + model_config = ConfigDict(populate_by_name=True) + + system_type: SystemType = Field( + default="skiers", + alias="system", + description=( + "Type of system. Allowed: 'skier', 'skiers', 'pst-', '-pst', 'rot', 'trans', 'vpst-', '-vpst'." + ), + )
59-61: Aliascut_lengthto legacycrack_lengthfor smoother migrationExamples and old configs may still use
crack_length.- cut_length: float = Field( - default=0.0, ge=0, description="Cut length of performed PST or VPST [mm]" - ) + cut_length: float = Field( + default=0.0, + ge=0, + alias="crack_length", + description="Cut length for PST/VPST [mm] (legacy alias: crack_length)", + )
67-72: Clarify no upper bound implied for surface_load; units are consistentText and non-negative constraint align with N/mm convention; consider stating “no upper bound enforced” if you’ve seen confusion in user configs.
tests/core/test_system_model.py (3)
325-328: Avoid in-place mutation of ScenarioConfig before update (copy first).Mutating shared config instances can cause subtle aliasing/caching issues. Copy, modify, then update.
- new_cfg = system.scenario.scenario_config - new_cfg.phi = 20.0 - system.update_scenario(scenario_config=new_cfg) + new_cfg = system.scenario.scenario_config.model_copy() + new_cfg.phi = 20.0 + system.update_scenario(scenario_config=new_cfg)
336-373: Add a follow-up test that toggling touchdown off resets solver args.You cover the True path; add a quick test to ensure (None, None, None) after disabling to catch cache/param leakage.
@@ def test_toggle_touchdown_switches_solver_arguments(self, mock_solve): """Test that toggle_touchdown switches the solver arguments.""" @@ self.assertEqual(calls[1], (900.0, "A_free_hanging", None)) + + @patch("weac.core.system_model.UnknownConstantsSolver.solve_for_unknown_constants") + def test_toggle_touchdown_off_resets_solver_arguments(self, mock_solve): + calls = [] + def solver_side_effect(scenario, eigensystem, system_type, + touchdown_distance, touchdown_mode, collapsed_weak_layer_kR): + calls.append((touchdown_distance, touchdown_mode, collapsed_weak_layer_kR)) + return np.zeros((6, len(scenario.segments))) + mock_solve.side_effect = solver_side_effect + system = self._build_model(touchdown=True, system_type="skiers") + with patch("weac.core.system_model.SlabTouchdown") as mock_td: + mock_inst = MagicMock() + mock_inst.touchdown_distance = 500.0 + mock_inst.touchdown_mode = "B_point_contact" + mock_inst.collapsed_weak_layer_kR = 1.0 + mock_td.return_value = mock_inst + _ = system.unknown_constants # with TD + system.toggle_touchdown(False) + _ = system.unknown_constants # recompute without TD + self.assertEqual(calls[0], (500.0, "B_point_contact", 1.0)) + self.assertEqual(calls[1], (None, None, None))
267-293: Also verify touchdown param threading for uncracked branch.Parity with the cracked path test will protect TD-arg propagation in the uncracked solver.
@@ def test_uncracked_unknown_constants_sets_all_foundation(self, mock_solve): """Test that uncracked_unknown_constants sets all foundation.""" - captured_scenarios = [] + captured_scenarios = [] + captured_args = [] @@ - captured_scenarios.append(scenario) + captured_scenarios.append(scenario) + captured_args.append((touchdown_distance, touchdown_mode, collapsed_weak_layer_kR)) n = len(scenario.segments) return np.zeros((6, n)) @@ self.assertTrue( all(seg.has_foundation for seg in captured_scenarios[-1].segments) ) + # Touchdown args should be None when touchdown is disabled + self.assertEqual(captured_args[-1], (None, None, None))tests/utils/weac_reference_runner.py (2)
114-132: Avoid code injection in version check; pass expected version via argv.Don’t embed untrusted version strings into executed code. Read from sys.argv instead.
- code = ( + code = ( "import sys\n" "try:\n" " from importlib.metadata import version, PackageNotFoundError\n" "except Exception:\n" " from importlib_metadata import version, PackageNotFoundError\n" "try:\n" - f" v = version('weac'); sys.exit(0 if v == '{version}' else 1)\n" + " expected = sys.argv[1]\n" + " v = version('weac'); sys.exit(0 if v == expected else 1)\n" "except PackageNotFoundError:\n" " sys.exit(2)\n" ) check_proc = subprocess.run( - [py_exe, "-c", code], + [py_exe, "-c", code, version], cwd=venv_dir, env=_clean_env(), check=False, )
49-53: Harden pip environment (disable version check) and always sanitize env.Minor speed/noise improvement; use PIP_DISABLE_PIP_VERSION_CHECK.
def _clean_env() -> Dict[str, str]: env = os.environ.copy() env.pop("PYTHONPATH", None) env["PYTHONNOUSERSITE"] = "1" + env.setdefault("PIP_DISABLE_PIP_VERSION_CHECK", "1") return envweac/core/system_model.py (1)
399-407: Vectorize z(...) for sequence x to reduce Python overhead.The current list-comprehension concatenation is allocation-heavy. Preallocate or batch-call if zh/zp can handle vector x.
- if isinstance(x, (list, tuple, np.ndarray)): - z = np.concatenate( - [ - np.dot(self.eigensystem.zh(xi, length, has_foundation), C) - + self.eigensystem.zp(xi, phi, has_foundation, qs) - for xi in x - ], - axis=1, - ) + if isinstance(x, (list, tuple, np.ndarray)): + x_arr = np.asarray(x) + cols = [] + for xi in x_arr: + cols.append( + self.eigensystem.zh(xi, length, has_foundation) @ C + + self.eigensystem.zp(xi, phi, has_foundation, qs) + ) + z = np.concatenate(cols, axis=1)weac/core/slab.py (2)
100-103: Vectorize boundary coordinate computation (O(n) instead of O(n²)).Replace repeated sum() with cumsum for clarity and performance.
- z = np.array([-H / 2 + sum(hi[0:j]) for j in range(n + 1)]) + z = -H / 2 + np.concatenate(([0.0], np.cumsum(hi)))
83-85: Clarify units of w (mass vs weight per unit width).Ai is [mm²] and rho is [t/mm³], so w is [t/mm]. Current doc implies [t] and “weight” (force), which is misleading.
- w : float - Weight of the slab segment that is cut off or added [t] + w : float + Mass per unit out-of-plane width of the triangular segment [t/mm] + (often interpreted as [t] assuming 1 mm width)weac/core/slab_touchdown.py (3)
250-255: Good: polynomial evaluation logging is at debug-levelThis avoids flooding logs during root-finding iterations.
329-335: Fix return docstring: it returns stiffness, not “has_foundation”Rename the return description to “stiffness” and clarify meaning.
Apply this diff:
- has_foundation : stiffness of substitute spring. + stiffness : substitute spring stiffness at x=0. + For dof='rot': kR = M/ψ. For dof='trans': kN = V/w. + UnknownConstantsSolver applies unit excitation, so numerically + kR=1/ψ and kN=1/w, but using the ratios keeps this robust.
350-363: Compute physical stiffness M/ψ and V/w; fix misleading commentsCurrent implementation returns 1/ψ and 1/w with comments that mention “moment of inertia” and “weight,” which are incorrect. Use the actual ratios M/ψ and V/w evaluated at x=0. This remains correct even if the unit excitation convention in UnknownConstantsSolver changes.
Apply this diff:
- stiffness = None - if dof in ["rot"]: - # For rotational stiffness: has_foundation = M / psi - # Uses M = 1.0 for the moment of inertia. - psi_val = fq.psi(z_at_x0)[0] # Extract scalar value from the result - stiffness = abs(1 / psi_val) if abs(psi_val) > 1e-12 else 1e12 - elif dof in ["trans"]: - # For translational stiffness: has_foundation = V / w - # Uses w = 1.0 for the weight of the slab. - w_val = fq.w(z_at_x0)[0] # Extract scalar value from the result - stiffness = abs(1 / w_val) if abs(w_val) > 1e-12 else 1e12 - if stiffness is None: - raise ValueError(f"Stiffness for {dof} is None") - return stiffness + stiffness = None + eps = 1e-12 + if dof in ["rot"]: + # Rotational substitute stiffness: kR = M / ψ at x=0 + psi_val = float(fq.psi(z_at_x0)[0]) + M_val = float(fq.M(z_at_x0)[0]) + stiffness = abs(M_val / psi_val) if abs(psi_val) > eps else 1e12 + elif dof in ["trans"]: + # Translational substitute stiffness: kN = V / w at x=0 + w_val = float(fq.w(z_at_x0)[0]) + V_val = float(fq.V(z_at_x0)[0]) + stiffness = abs(V_val / w_val) if abs(w_val) > eps else 1e12 + else: + raise ValueError(f"Invalid dof: {dof}. Expected 'rot' or 'trans'.") + return stiffness
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
.github/workflows/pylint.yml(1 hunks).github/workflows/tests.yml(1 hunks)README.md(5 hunks)tests/core/test_system_model.py(1 hunks)tests/run_tests.py(1 hunks)tests/test_comparison_results.py(1 hunks)tests/utils/test_misc.py(1 hunks)tests/utils/weac_reference_runner.py(1 hunks)weac/components/scenario_config.py(1 hunks)weac/core/slab.py(1 hunks)weac/core/slab_touchdown.py(1 hunks)weac/core/system_model.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
tests/utils/test_misc.py (1)
weac/utils/misc.py (2)
decompose_to_normal_tangential(13-38)get_skier_point_load(41-56)
weac/core/slab.py (1)
weac/components/layer.py (1)
Layer(97-171)
tests/run_tests.py (1)
weac/logging_config.py (1)
setup_logging(10-39)
tests/utils/weac_reference_runner.py (1)
weac/core/system_model.py (1)
z(366-413)
tests/core/test_system_model.py (5)
weac/components/layer.py (2)
Layer(97-171)WeakLayer(174-276)weac/components/model_input.py (1)
ModelInput(28-76)weac/components/scenario_config.py (1)
ScenarioConfig(15-72)weac/components/segment.py (1)
Segment(8-31)weac/core/system_model.py (10)
SystemModel(38-413)eigensystem(152-155)unknown_constants(198-256)update_layers(306-316)update_weak_layer(294-303)update_scenario(319-341)slab_touchdown(158-195)uncracked_unknown_constants(259-291)toggle_touchdown(343-348)z(366-413)
weac/core/system_model.py (9)
weac/components/config.py (1)
Config(16-28)weac/components/layer.py (2)
Layer(97-171)WeakLayer(174-276)weac/components/model_input.py (1)
ModelInput(28-76)weac/components/segment.py (1)
Segment(8-31)weac/core/eigensystem.py (3)
Eigensystem(20-405)zh(218-274)zp(276-358)weac/core/field_quantities.py (1)
FieldQuantities(35-273)weac/core/scenario.py (1)
Scenario(17-200)weac/core/slab.py (1)
Slab(13-149)weac/core/unknown_constants_solver.py (2)
UnknownConstantsSolver(28-444)solve_for_unknown_constants(34-256)
tests/test_comparison_results.py (9)
weac/analysis/analyzer.py (7)
Analyzer(50-790)rasterize_solution(95-176)get_zmesh(179-225)Sxx(228-281)Txz(284-339)Szz(342-399)principal_stress_slab(402-469)weac/components/layer.py (2)
Layer(97-171)WeakLayer(174-276)weac/components/model_input.py (1)
ModelInput(28-76)weac/components/scenario_config.py (1)
ScenarioConfig(15-72)weac/components/segment.py (1)
Segment(8-31)weac/components/config.py (1)
Config(16-28)weac/core/system_model.py (4)
SystemModel(38-413)unknown_constants(198-256)z(366-413)slab_touchdown(158-195)tests/utils/weac_reference_runner.py (1)
compute_reference_model_results(287-379)weac/core/field_quantities.py (1)
Gi(126-139)
weac/core/slab_touchdown.py (7)
weac/components/layer.py (1)
WeakLayer(174-276)weac/components/scenario_config.py (1)
ScenarioConfig(15-72)weac/components/segment.py (1)
Segment(8-31)weac/core/eigensystem.py (3)
Eigensystem(20-405)zh(218-274)zp(276-358)weac/core/field_quantities.py (3)
FieldQuantities(35-273)psi(76-83)w(68-70)weac/core/scenario.py (1)
Scenario(17-200)weac/core/unknown_constants_solver.py (2)
UnknownConstantsSolver(28-444)solve_for_unknown_constants(34-256)
🪛 markdownlint-cli2 (0.17.2)
README.md
37-37: Inline HTML
Element: a
(MD033, no-inline-html)
123-123: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
🪛 LanguageTool
README.md
[grammar] ~122-~122: There might be a mistake here.
Context: ...om/2phi/weac/blob/main/pyproject.toml)): - [Python](https://www.python.org/downloads...
(QB_NEW_EN)
[grammar] ~123-~123: There might be a mistake here.
Context: ...ownloads/release/python-3100/) ≥ 3.10 - Numpy ≥ 2.0.1 - ...
(QB_NEW_EN)
[grammar] ~124-~124: There might be a mistake here.
Context: ...- Numpy ≥ 2.0.1 - Scipy ≥ 1.14...
(QB_NEW_EN)
[grammar] ~125-~125: There might be a mistake here.
Context: ...ipy](https://www.scipy.org/) ≥ 1.14.0 - Matplotlib &ge...
(QB_NEW_EN)
[grammar] ~126-~126: There might be a mistake here.
Context: ...lib](https://matplotlib.org/) ≥ 3.9.1 - [Pydantic](https://docs.pydantic.dev/late...
(QB_NEW_EN)
[grammar] ~127-~127: There might be a mistake here.
Context: ...//docs.pydantic.dev/latest/) ≥ 2.11.7 - [Snowpylot](https://github.com/connellymk...
(QB_NEW_EN)
[grammar] ~128-~128: Use correct spacing
Context: ...hub.com/connellymk/snowpylot) ≥ 1.1.3 ## Usage The following describes the basic...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~131-~131: Use correct spacing
Context: ... 1.1.3 ## Usage The following describes the basic usage ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~133-~133: Use correct spacing
Context: ...tps://2phi.github.io/weac/) for details. Load the module. ```python import weac ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~135-~135: Use correct spacing
Context: ...io/weac/) for details. Load the module. python import weac Choose a snow profile from the preconfig...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~141-~141: Use correct spacing
Context: ...w surface) to bottom (above weak layer). python from weac.components import Layer layers = [ Layer(rho=170, h=100), # (1) surface layer Layer(rho=190, h=40), # (2) Layer(rho=230, h=130), # : Layer(rho=250, h=20), Layer(rho=210, h=70), Layer(rho=380, h=20), # : Layer(rho=280, h=100) # (N) last slab layer above weak layer ] Create a WeakLayer instance that lies un...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~157-~157: Use correct spacing
Context: ... instance that lies underneath the slab. python from weac.components import WeakLayer weak_layer = WeakLayer(rho=125, h=20) Create a Scenario that defines the envir...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~165-~165: There might be a mistake here.
Context: ...environment and setup that the slab and weaklayer will be evaluated in. ```python from w...
(QB_NEW_EN_OTHER)
[grammar] ~165-~165: Use correct spacing
Context: ...slab and weaklayer will be evaluated in. python from weac.components import ScenarioConfig, Segment # Example 1: SKIER skier_config = ScenarioConfig( system_type='skier', phi=30, ) skier_segments = [ Segment(length=5000, has_foundation=True, m=0), Segment(length=0, has_foundation=False, m=80), Segment(length=0, has_foundation=False, m=0), Segment(length=5000, has_foundation=True, m=0), ] # Scenario is a skier of 80 kg standing on a 10 meter long slab at a 30 degree angle # Exampel 2: PST pst_config = ScenarioConfig( system_type='pst-', # Downslope cut phi=30, # (counterclockwise positive) cut_length=300, ) pst_segments = [ Segment(length=5000, has_foundation=True, m=0), Segment(length=300, has_foundation=False, m=0), # Crack Segment ] # Scenario is Downslope PST with a 300mm cut Create SystemModel instance that combine...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[style] ~193-~193: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... is Downslope PST with a 300mm cut ``` Create SystemModel instance that combines the ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~194-~194: Use correct spacing
Context: ...m solving and field quantity extraction. python from weac.components import Config, ModelInput from weac.core.system_model import SystemModel # Example: build a model for the skier scenario defined above model_input = ModelInput( weak_layer=weak_layer, scenario_config=skier_config, layers=custom_layers, segments=skier_segments, ) system_config = Config( touchdown=True ) skier_system = SystemModel( model_input=model_input, config=system_config, ) Unknown constants are cached_properties;...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~216-~216: There might be a mistake here.
Context: ...em_config, ) ``` Unknown constants are cached_properties; calling `skier_system.unknown_constants...
(QB_NEW_EN_OTHER)
[grammar] ~216-~216: There might be a problem here.
Context: ...ar equations and extracts the constants. python C = skier_system.unknown_constants Analyzer handles rasterization + computation of ...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~222-~222: There might be a mistake here.
Context: ...weak-layer properties Sxx, Sxz, etc. Prepare the output by rasterizing the so...
(QB_NEW_EN)
[grammar] ~223-~223: Use correct spacing
Context: ...ates that are supported by a foundation. python from weac.analysis.analyzer import Analyzer skier_analyzer = Analyzer(skier_system) xsl_skier, z_skier, xwl_skier = skier_analyzer.rasterize_solution(mode="cracked") Gdif, GdifI, GdifII = skier_analyzer.differential_ERR() Ginc, GincI, GincII = skier_analyzer.incremental_ERR() # and Sxx, Sxz, Tzz, principal stress, incremental_potential, ... Visualize the results. ```python from w...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~235-~235: Use correct spacing
Context: ...tential, ... Visualize the results. python from weac.analysis.plotter import Plotter plotter = Plotter() # Visualize slab profile fig = plotter.plot_slab_profile( weak_layers=weak_layer, slabs=skier_system.slab, ) # Visualize deformations as a contour plot fig = plotter.plot_deformed( xsl_skier, xwl_skier, z_skier, skier_analyzer, scale=200, window=200, aspect=2, field="Sxx" ) # Plot slab displacements (using x-coordinates of all segments, xsl) plotter.plot_displacements(skier_analyzer, x=xsl_skier, z=z_skier) # Plot weak-layer stresses (using only x-coordinates of bedded segments, xwl) plotter.plot_stresses(skier_analyzer, x=xwl_skier, z=z_skier) ``` Compute output/field quantities for expo...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~258-~258: Use correct spacing
Context: ...ld quantities for exporting or plotting. python # Compute stresses in kPa in the weaklayer tau = skier_system.fq.tau(Z=z_skier, unit='kPa') sig = skier_system.fq.sig(Z=z_skier, unit='kPa') w = skier_system.fq.w(Z=z_skier, unit='um') # Example evaluation vertical displacement at top/mid/bottom of the slab u_top = skier_system.fq.u(Z=z_skier, h0=top, unit='um') u_mid = skier_system.fq.u(Z=z_skier, h0=mid, unit='um') u_bot = skier_system.fq.u(Z=z_skier, h0=bot, unit='um') psi = skier_system.fq.psi(Z=z_skier, unit='deg') ## Roadmap See the [open issues](https://g...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~274-~274: Use correct spacing
Context: ...='deg') ``` ## Roadmap See the [open issues](https://github.com...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~276-~276: Use correct spacing
Context: ...t of proposed features and known issues. ### v4.0 - [] Change to scenario & scenario...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~278-~278: Use correct spacing
Context: ...sed features and known issues. ### v4.0 - [] Change to scenario & scenario_config:...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~280-~280: Use correct spacing
Context: ...enario_config: InfEnd/Cut/Segment/Weight ### v3.2 <!-- - [ ] New mathematical foundat...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~282-~282: Use correct spacing
Context: ...fig: InfEnd/Cut/Segment/Weight ### v3.2 - [ ] Complex terrain through the addition...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~284-~284: There might be a mistake here.
Context: ...hrough the addition of out-of-plane tilt - [ ] Up, down, and cross-slope cracks ##...
(QB_NEW_EN)
[grammar] ~285-~285: Use correct spacing
Context: ...t - [ ] Up, down, and cross-slope cracks ### v3.1 - [ ] Improved CriteriaEvaluator O...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~287-~287: Use correct spacing
Context: ..., down, and cross-slope cracks ### v3.1 - [ ] Improved CriteriaEvaluator Optimizat...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~289-~289: Use correct spacing
Context: ...aluator Optimization (x2 time reduction) ## Release history ### v3.0 - Code Refact...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~291-~291: Use correct spacing
Context: ... (x2 time reduction) ## Release history ### v3.0 - Code Refactor - Input Validation...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~293-~293: Use correct spacing
Context: ...reduction) ## Release history ### v3.0 - Code Refactor - Input Validation - Modul...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~295-~295: There might be a mistake here.
Context: ...lease history ### v3.0 - Code Refactor - Input Validation - Modular + Object-Orie...
(QB_NEW_EN)
[grammar] ~296-~296: There might be a mistake here.
Context: ...v3.0 - Code Refactor - Input Validation - Modular + Object-Oriented ### v2.6 - F...
(QB_NEW_EN)
[grammar] ~297-~297: Use correct spacing
Context: ...t Validation - Modular + Object-Oriented ### v2.6 - Finite fracture mechanics implem...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~299-~299: Use correct spacing
Context: ...on - Modular + Object-Oriented ### v2.6 - Finite fracture mechanics implementation...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~303-~303: Use correct spacing
Context: ...tropic weak layer (?) - Add demo gif (?) ### v2.5 - Analyze slab touchdown in PST ex...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~305-~305: Use correct spacing
Context: ...k layer (?) - Add demo gif (?) ### v2.5 - Analyze slab touchdown in PST experiment...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~308-~308: There might be a mistake here.
Context: ...significantly improved API documentation ### v2.4 - Choose between slope-normal (`'-...
(QB_NEW_EN_OTHER)
[grammar] ~310-~310: Use correct spacing
Context: ...tly improved API documentation ### v2.4 - Choose between slope-normal ('-pst', `...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~312-~312: There might be a mistake here.
Context: ...st', 'vpst-'`) PST boundary conditions ### v2.3 - Stress plots on deformed contour...
(QB_NEW_EN_OTHER)
[grammar] ~314-~314: Use correct spacing
Context: ...st-'`) PST boundary conditions ### v2.3 - Stress plots on deformed contours - PSTs...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~316-~316: There might be a mistake here.
Context: ...s ### v2.3 - Stress plots on deformed contours - PSTs now account for slab touchdown ###...
(QB_NEW_EN_OTHER)
[grammar] ~317-~317: There might be a mistake here.
Context: ...rs - PSTs now account for slab touchdown ### v2.2 - Sign of inclination phi consis...
(QB_NEW_EN_OTHER)
[grammar] ~319-~319: Use correct spacing
Context: ...now account for slab touchdown ### v2.2 - Sign of inclination phi consistent wit...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~331-~331: There might be a mistake here.
Context: ...t use of coordinate system with downward pointing z-axis - Consitent top-to-botto...
(QB_NEW_EN_OTHER)
[grammar] ~331-~331: Ensure spelling is correct
Context: ...ate system with downward pointing z-axis - Consitent top-to-bottom numbering of slab layers ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~332-~332: There might be a mistake here.
Context: ...t top-to-bottom numbering of slab layers - Implementation of PSTs cut from either l...
(QB_NEW_EN)
[grammar] ~333-~333: There might be a mistake here.
Context: ...ayers - Implementation of PSTs cut from either left or right side ### v2.0 - Complet...
(QB_NEW_EN)
[grammar] ~333-~333: Use correct spacing
Context: ... PSTs cut from either left or right side ### v2.0 - Completely rewritten in 🐍 Pytho...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~335-~335: Use correct spacing
Context: ...from either left or right side ### v2.0 - Completely rewritten in 🐍 Python - Coup...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~360-~360: There might be a mistake here.
Context: ...ork the project 2. Initialize submodules bash git submodule update --init --recursive 3. Create your feature branch (`git checkou...
(QB_NEW_EN_OTHER)
[grammar] ~366-~366: There might be a mistake here.
Context: ...git checkout -b feature/amazingfeature) 4. Commit your changes (git commit -m 'Add...
(QB_NEW_EN_OTHER)
[style] ~367-~367: Consider using a more formal and expressive alternative to ‘amazing’.
Context: ...mazingfeature) 4. Commit your changes (git commit -m 'Add some amazing feature') 5. Push to the branch (git ...
(AWESOME)
[grammar] ~367-~367: There might be a mistake here.
Context: ...t commit -m 'Add some amazing feature') 5. Push to the branch (git push origin fea...
(QB_NEW_EN_OTHER)
[grammar] ~369-~369: There might be a mistake here.
Context: .../amazingfeature`) 6. Open a pull request ## Workflows [![Publish Python 🐍 releases ...
(QB_NEW_EN_OTHER)
🔇 Additional comments (7)
.github/workflows/pylint.yml (1)
19-24: Nice: pip cache enabled on setup-pythonThis will reduce CI time by reusing downloads.
tests/utils/test_misc.py (1)
15-17: Solid, comprehensive coverage for decomposition and load utilitiesGood breadth of cases (0°, 90°, 45°, ±φ symmetry, obtuse, array inputs, energy conservation) and clear assertions. This will catch sign and unit regressions early.
tests/run_tests.py (1)
50-59: Good: success-rate guard avoids division by zeroThanks for guarding
testsRun == 0; avoids ZeroDivisionError when discovery yields no tests.weac/components/scenario_config.py (1)
53-58: Bounds for phi look good (±90°) and description is clearCovers steep and near-vertical slopes and documents sign convention.
weac/core/slab_touchdown.py (3)
29-39: Docstring typo from earlier review is fixed; goodThe “is strictly smaller than the cut length” correction is in place and reads clearly now.
87-95: Good: flat config cloned correctly with phi=0You preserve system_type, cut_length, stiffness_ratio, and surface_load while forcing phi=0. This keeps substitute-stiffness problems on a flat configuration, which is appropriate.
337-339: Fix invalid segment position key in UnknownConstantsSolverThe first element of the position vector is being set to
"length", which isn’t one of the acceptedposvalues ('l'|'left','m'|'mid','r'|'right'). This will skip all branches in_setup_conditionsand cause a crash at runtime. Update the assignment to use a supported key:• File:
weac/core/unknown_constants_solver.py
Location (around line 79):- pi[0], pi[-1] = "length", "r" + pi[0], pi[-1] = "l", "r"With this change, the first segment will correctly be treated as the left boundary (
'l'), matching the checks in_setup_conditions.Likely an incorrect or invalid review comment.
| # Unpack variables | ||
| bs = -(self.eigensystem.B11**2 / self.eigensystem.A11 - self.eigensystem.D11) | ||
| ss = self.eigensystem.kA55 | ||
| L = self.scenario.L | ||
| cut_length = self.scenario.cut_length | ||
| crack_h = self.scenario.crack_h | ||
| qn = self.scenario.qn | ||
|
|
||
| # Spring stiffness of uncollapsed eigensystem of length L - cut_length | ||
| straight_scenario = self._generate_straight_scenario(L - cut_length) | ||
| kRl = self._substitute_stiffness(straight_scenario, self.eigensystem, "rot") | ||
| kNl = self._substitute_stiffness(straight_scenario, self.eigensystem, "trans") | ||
|
|
There was a problem hiding this comment.
Guard for qn≈0 and robust bracketing for touchdown_distance in mode C
- If qn≈0, coefficients collapse and brentq will fail.
- The fixed bracket [cut/1000, 0.999*cut] can miss sign changes; add adaptive bracketing.
- Also guard against cut_length ≤ 0.
Add early checks and bracket scanning like above.
Apply this diff:
# Unpack variables
bs = -(self.eigensystem.B11**2 / self.eigensystem.A11 - self.eigensystem.D11)
ss = self.eigensystem.kA55
L = self.scenario.L
cut_length = self.scenario.cut_length
crack_h = self.scenario.crack_h
qn = self.scenario.qn
+ eps = 1e-12
+ if cut_length <= 0:
+ raise ValueError(f"cut_length must be > 0 for mode C, got {cut_length}")
+ if abs(qn) <= eps:
+ raise ValueError(f"Normal load qn≈0 (qn={qn:.3e}); touchdown_distance undefined.")
@@
- # Find root
- touchdown_distance = brentq(
- polynomial, cut_length / 1000, 999 / 1000 * cut_length
- )
+ # Find root with robust bracketing
+ a, b = cut_length / 1000.0, 0.999 * cut_length
+ fa, fb = polynomial(a), polynomial(b)
+ if fa == 0.0:
+ return a
+ if fb == 0.0:
+ return b
+ if fa * fb > 0:
+ N = 96
+ prev_x, prev_f = a, fa
+ found = False
+ for i in range(1, N + 1):
+ x = a + (b - a) * i / N
+ fval = polynomial(x)
+ if prev_f * fval <= 0:
+ a, b = prev_x, x
+ found = True
+ break
+ prev_x, prev_f = x, fval
+ if not found:
+ raise ValueError("Failed to bracket a root for touchdown_distance in (0, cut_length).")
+ touchdown_distance = brentq(polynomial, a, b)Also applies to: 286-290
🤖 Prompt for AI Agents
In weac/core/slab_touchdown.py around lines 237-249 (and similarly at 286-290),
add guards and robust bracketing: first validate scenario.cut_length > 0 and
raise/return early if not; then detect qn ≈ 0 using np.isclose(self.scenario.qn,
0) (or an absolute tolerance) and handle that branch explicitly to avoid forming
degenerate coefficients (e.g., skip root-finding or set touchdown_distance to a
safe default). Replace the fixed brentq bracket [cut/1000, 0.999*cut] with an
adaptive scan that samples a sequence of points from a small positive fraction
up to just below cut_length, searching for an interval where the function
changes sign and using that interval for brentq; if no sign change is found,
fall back to a safe behavior (raise a clear exception or set a fallback value)
instead of calling brentq with an invalid bracket.
| def _generate_straight_scenario(self, L: float) -> Scenario: | ||
| """ | ||
| Generate a straight scenario with a given length. | ||
| """ | ||
| segments = [Segment(length=L, has_foundation=True, m=0)] | ||
| straight_scenario = Scenario( | ||
| scenario_config=self.flat_config, | ||
| segments=segments, | ||
| weak_layer=self.scenario.weak_layer, | ||
| slab=self.scenario.slab, | ||
| ) | ||
| return straight_scenario | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Minor: declared class attribute ‘straight_scenario’ is never set
You declare straight_scenario in the class attributes list but only use a local variable inside _generate_straight_scenario. Either set self.straight_scenario or drop the class attribute to avoid confusion.
Apply this diff to remove the unused class attribute:
- straight_scenario: ScenarioCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In weac/core/slab_touchdown.py around lines 305 to 317, the class declares a
straight_scenario attribute but the method _generate_straight_scenario only
creates a local straight_scenario variable, leaving the class attribute unused;
either assign the generated Scenario to self.straight_scenario
(self.straight_scenario = straight_scenario) so the instance attribute is
populated, or remove the unused class attribute declaration from the class
definition to avoid confusion—pick the option that matches intended design and
update references accordingly.
| unknown_constants = UnknownConstantsSolver.solve_for_unknown_constants( | ||
| scenario=scenario, eigensystem=eigensystem, system_type=dof | ||
| ) | ||
|
|
||
| # Calculate field quantities at x=0 (left end) | ||
| Zh0 = eigensystem.zh(x=0, length=scenario.L, has_foundation=True) | ||
| zp0 = eigensystem.zp(x=0, phi=0, has_foundation=True, qs=0) | ||
| C_at_x0 = unknown_constants[:, 0].reshape(-1, 1) # Ensure column vector | ||
| z_at_x0 = Zh0 @ C_at_x0 + zp0 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap UnknownConstantsSolver failures with context
If the linear system is singular or ill-conditioned, propagate a clear error from this context to aid callers (especially during root finding).
Apply this diff:
- unknown_constants = UnknownConstantsSolver.solve_for_unknown_constants(
- scenario=scenario, eigensystem=eigensystem, system_type=dof
- )
+ try:
+ unknown_constants = UnknownConstantsSolver.solve_for_unknown_constants(
+ scenario=scenario, eigensystem=eigensystem, system_type=dof
+ )
+ except Exception as e:
+ raise ValueError(f"Failed to compute substitute stiffness (dof='{dof}', L={scenario.L:.3f}): {e}") from e📝 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.
| unknown_constants = UnknownConstantsSolver.solve_for_unknown_constants( | |
| scenario=scenario, eigensystem=eigensystem, system_type=dof | |
| ) | |
| # Calculate field quantities at x=0 (left end) | |
| Zh0 = eigensystem.zh(x=0, length=scenario.L, has_foundation=True) | |
| zp0 = eigensystem.zp(x=0, phi=0, has_foundation=True, qs=0) | |
| C_at_x0 = unknown_constants[:, 0].reshape(-1, 1) # Ensure column vector | |
| z_at_x0 = Zh0 @ C_at_x0 + zp0 | |
| try: | |
| unknown_constants = UnknownConstantsSolver.solve_for_unknown_constants( | |
| scenario=scenario, eigensystem=eigensystem, system_type=dof | |
| ) | |
| except Exception as e: | |
| raise ValueError( | |
| f"Failed to compute substitute stiffness (dof='{dof}', L={scenario.L:.3f}): {e}" | |
| ) from e | |
| # Calculate field quantities at x=0 (left end) | |
| Zh0 = eigensystem.zh(x=0, length=scenario.L, has_foundation=True) | |
| zp0 = eigensystem.zp(x=0, phi=0, has_foundation=True, qs=0) | |
| C_at_x0 = unknown_constants[:, 0].reshape(-1, 1) # Ensure column vector | |
| z_at_x0 = Zh0 @ C_at_x0 + zp0 |
🤖 Prompt for AI Agents
In weac/core/slab_touchdown.py around lines 337 to 346, wrap the call to
UnknownConstantsSolver.solve_for_unknown_constants in a try/except that catches
linear-algebra errors (e.g. np.linalg.LinAlgError, numpy.linalg.LinAlgError, or
a broad Exception if the solver raises its own) and re-raise a new RuntimeError
(or a custom exception) with clear context including scenario identifier (e.g.
scenario.name or scenario.L), dof/system_type, and that the linear system is
singular or ill-conditioned, while preserving the original exception as the
__cause__ or by including its message; keep the later usage of unknown_constants
unchanged so callers receive a contextual error during root-finding.
| We utilize the pydantic library to define the system model. | ||
| """ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Docstring nit: SystemModel itself is not a Pydantic model.
Pydantic is used for component models; SystemModel is a plain orchestrator class. Clarify to avoid confusion.
-We utilize the pydantic library to define the system model.
+Pydantic is used for component models (inputs/config). SystemModel orchestrates them.📝 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.
| We utilize the pydantic library to define the system model. | |
| """ | |
| Pydantic is used for component models (inputs/config). SystemModel orchestrates them. | |
| """ |
🤖 Prompt for AI Agents
In weac/core/system_model.py around lines 8 to 9, the module docstring
incorrectly implies SystemModel is a Pydantic model; clarify that Pydantic is
used only for component models and that SystemModel is a plain
orchestrator/class that composes those Pydantic component models. Update the
docstring to state that Pydantic models define components (e.g., ComponentA,
ComponentB) while SystemModel orchestrates and validates/holds them but does not
inherit from pydantic.BaseModel.
| @cached_property | ||
| def slab_touchdown(self) -> Optional[SlabTouchdown]: | ||
| """ | ||
| Solve for the slab touchdown. | ||
| Modifies the scenario object in place by replacing the undercut segment | ||
| with a new segment of length equal to the touchdown distance if the system is | ||
| a PST or VPST. | ||
| """ | ||
| if self.config.touchdown: | ||
| logger.info("Solving for Slab Touchdown") | ||
| slab_touchdown = SlabTouchdown( | ||
| scenario=self.scenario, eigensystem=self.eigensystem | ||
| ) | ||
| logger.info( | ||
| "Original cut_length: %s, touchdown_distance: %s", | ||
| self.scenario.cut_length, | ||
| slab_touchdown.touchdown_distance, | ||
| ) | ||
|
|
||
| new_segments = copy.deepcopy(self.scenario.segments) | ||
| if self.scenario.system_type in ("pst-", "vpst-"): | ||
| new_segments[-1].length = slab_touchdown.touchdown_distance | ||
| elif self.scenario.system_type in ("-pst", "-vpst"): | ||
| new_segments[0].length = slab_touchdown.touchdown_distance | ||
|
|
||
| # Create new scenario with updated segments | ||
| self.scenario = Scenario( | ||
| scenario_config=self.scenario.scenario_config, | ||
| segments=new_segments, | ||
| weak_layer=self.weak_layer, | ||
| slab=self.slab, | ||
| ) | ||
| logger.info( | ||
| "Updated scenario with new segment lengths: %s", | ||
| [seg.length for seg in new_segments], | ||
| ) | ||
|
|
||
| return slab_touchdown | ||
| return None |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Cached property with side effects (mutating self.scenario).
Accessing slab_touchdown replaces self.scenario lengths. It’s documented, but still surprising for a cached property. Consider isolating mutation behind an explicit method (e.g., apply_touchdown) or computing an effective Scenario for solves.
Please confirm this side effect is acceptable across analyzers/plotters and any external consumers. If not, I can draft an “effective_scenario” pattern that keeps self.scenario immutable.
🤖 Prompt for AI Agents
In weac/core/system_model.py around lines 157 to 195, slab_touchdown is a
cached_property that mutates self.scenario by replacing segment lengths; change
this so the cached property does not have side effects: extract the mutation
logic into an explicit method apply_touchdown(scenario) that returns a new
Scenario (or implement a cached effective_scenario property that returns a
modified copy) and have slab_touchdown only compute and return the SlabTouchdown
without altering self.scenario; update all internal callers (analyzers/plotters)
to use the explicit apply_touchdown() or effective_scenario when they need the
modified segments, and remove in-place assignment to self.scenario to keep the
original scenario immutable.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
.github/workflows/tests.yml (1)
30-31: Verify tests fail the job on failures (non-zero exit)Confirm tests/run_tests.py exits non-zero on failure. Earlier commits didn’t; your latest changes may have fixed it—let’s verify.
#!/bin/bash set -euo pipefail # Check that the test runner exits non-zero on failure # 1) Ensure a sys.exit based on wasSuccessful() exists in __main__ rg -n -C2 --type=py 'if __name__ == .__main__.:|sys\.exit\(|wasSuccessful\(' tests/run_tests.py || true # 2) Heuristic: look for unittest result used in sys.exit rg -n -C2 --type=py 'result\s*=\s*run_tests\(\)' tests/run_tests.py || trueIf absent, update main:
if __name__ == "__main__": import sys result = run_tests() sys.exit(0 if result.wasSuccessful() else 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/tests.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/tests.yml (1)
1-11: Triggers and YAML formatting look goodQuoting "on", fixing indentation under steps, and ending with a newline are all addressed. Workflow triggers cover PRs, workflow_call, and manual dispatch appropriately.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (10)
README.md (5)
184-194: Fix header typo: “Exampel 2” → “Example 2”.Keep example headers clean and searchable.
-# Exampel 2: PST +# Example 2: PST
362-366: Remove trailing spaces in fenced code block language tag.The two spaces after “```bash” violate markdownlint; keep indentation and surrounding blank lines as-is.
- ```bash + ```bash git submodule update --init --recursive ```
203-208: Fix undefined variable in ModelInput: uselayers(defined above), notcustom_layers.As written, the snippet will error at runtime.
model_input = ModelInput( weak_layer=weak_layer, scenario_config=skier_config, - layers=custom_layers, + layers=layers, segments=skier_segments, )
218-218: Wording: “cached_properties” → “cached properties”.Also avoid backticks unless referencing a symbol.
-Unknown constants are cached_properties; calling `skier_system.unknown_constants` solves the system of linear equations and extracts the constants. +Unknown constants are cached properties; calling `skier_system.unknown_constants` solves the system of linear equations and extracts the constants.
263-273: Fix terminology and undefined variables (top,mid,bot).
- “weaklayer” → “weak layer” in the comment.
- Replace
top/mid/botwith concrete values or derive fromweak_layer.hso the snippet runs.-# Compute stresses in kPa in the weaklayer +# Compute stresses in kPa in the weak layer tau = skier_system.fq.tau(Z=z_skier, unit='kPa') sig = skier_system.fq.sig(Z=z_skier, unit='kPa') w = skier_system.fq.w(Z=z_skier, unit='um') -# Example evaluation vertical displacement at top/mid/bottom of the slab -u_top = skier_system.fq.u(Z=z_skier, h0=top, unit='um') -u_mid = skier_system.fq.u(Z=z_skier, h0=mid, unit='um') -u_bot = skier_system.fq.u(Z=z_skier, h0=bot, unit='um') +# Example: vertical displacement at top/mid/bottom of the weak layer (mm from top surface) +u_top = skier_system.fq.u(Z=z_skier, h0=0.0, unit='um') +u_mid = skier_system.fq.u(Z=z_skier, h0=weak_layer.h/2, unit='um') +u_bot = skier_system.fq.u(Z=z_skier, h0=weak_layer.h, unit='um') psi = skier_system.fq.psi(Z=z_skier, unit='deg').github/workflows/tests.yml (5)
4-11: Good fix: quoted “on” and clean trigger sectionQuoting the on key and structuring triggers looks correct now. Previous yamllint complaints about truthy “on” are resolved.
4-8: Optionally scope PR triggers to main/developIf you want to limit CI runs to primary branches (as hinted earlier), add a branches filter.
Apply:
pull_request: types: [opened, reopened, synchronize, ready_for_review] + branches: [main, develop]
11-13: Harden workflow: least-privilege token + cancel superseded runsReduce token scope and cancel in-flight runs on new pushes to the same PR.
Apply:
workflow_dispatch: +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + jobs:
28-31: Install build tooling and test extras (if defined)
- Ensure setuptools/wheel are present for PEP 517 builds.
- If you define dev/test extras in pyproject, install them to pull test-only deps; otherwise keep -e . or use requirements.txt.
Option A (preferred if extras exist):
- name: Install dependencies run: | - python -m pip install --upgrade pip - python -m pip install -e . + python -m pip install --upgrade pip setuptools wheel + python -m pip install -e ".[dev]"Option B (fallback if no extras; use requirements.txt):
- name: Install dependencies run: | - python -m pip install --upgrade pip - python -m pip install -e . + python -m pip install --upgrade pip setuptools wheel + python -m pip install -e . + python -m pip install -r requirements.txt
25-27: Include requirements.txt in cache key to improve cache hitsIf you keep a requirements.txt (added in this PR), include it so cache invalidates when deps change there.
Apply:
cache-dependency-path: | pyproject.toml + requirements.txt
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/tests.yml(1 hunks)README.md(5 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
37-37: Inline HTML
Element: a
(MD033, no-inline-html)
131-131: Multiple consecutive blank lines
Expected: 1; Actual: 2
(MD012, no-multiple-blanks)
🪛 LanguageTool
README.md
[grammar] ~122-~122: Use correct spacing
Context: ...om/2phi/weac/blob/main/pyproject.toml)): - [Python](https://www.python.org/downloads...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~124-~124: There might be a mistake here.
Context: ...g/downloads/release/python-3120/) ≥ 3.12 - Numpy ≥ 2.0.1 - [Sc...
(QB_NEW_EN)
[grammar] ~125-~125: There might be a mistake here.
Context: ...12 - Numpy ≥ 2.0.1 - Scipy ≥ 1.14.0 ...
(QB_NEW_EN)
[grammar] ~126-~126: There might be a mistake here.
Context: ...Scipy ≥ 1.14.0 - Matplotlib ≥ 3...
(QB_NEW_EN)
[grammar] ~127-~127: There might be a mistake here.
Context: ...lotlib](https://matplotlib.org/) ≥ 3.9.1 - [Pydantic](https://docs.pydantic.dev/late...
(QB_NEW_EN)
[grammar] ~128-~128: There might be a mistake here.
Context: ...ps://docs.pydantic.dev/latest/) ≥ 2.11.7 - [Snowpylot](https://github.com/connellymk...
(QB_NEW_EN)
[grammar] ~129-~129: Use correct spacing
Context: ...github.com/connellymk/snowpylot) ≥ 1.1.3 ## Usage The following describes the basic...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~133-~133: Use correct spacing
Context: ...1.1.3 ## Usage The following describes the basic usage ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~135-~135: Use correct spacing
Context: ...tps://2phi.github.io/weac/) for details. Load the module. ```python import weac ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~137-~137: Use correct spacing
Context: ...io/weac/) for details. Load the module. python import weac Choose a snow profile from the preconfig...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~143-~143: Use correct spacing
Context: ...w surface) to bottom (above weak layer). python from weac.components import Layer layers = [ Layer(rho=170, h=100), # (1) surface layer Layer(rho=190, h=40), # (2) Layer(rho=230, h=130), # : Layer(rho=250, h=20), Layer(rho=210, h=70), Layer(rho=380, h=20), # : Layer(rho=280, h=100) # (N) last slab layer above weak layer ] Create a WeakLayer instance that lies un...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~159-~159: Use correct spacing
Context: ... instance that lies underneath the slab. python from weac.components import WeakLayer weak_layer = WeakLayer(rho=125, h=20) Create a Scenario that defines the envir...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~167-~167: There might be a mistake here.
Context: ...environment and setup that the slab and weaklayer will be evaluated in. ```python from w...
(QB_NEW_EN_OTHER)
[grammar] ~167-~167: Use correct spacing
Context: ...slab and weaklayer will be evaluated in. python from weac.components import ScenarioConfig, Segment # Example 1: SKIER skier_config = ScenarioConfig( system_type='skier', phi=30, ) skier_segments = [ Segment(length=5000, has_foundation=True, m=0), Segment(length=0, has_foundation=False, m=80), Segment(length=0, has_foundation=False, m=0), Segment(length=5000, has_foundation=True, m=0), ] # Scenario is a skier of 80 kg standing on a 10 meter long slab at a 30 degree angle # Exampel 2: PST pst_config = ScenarioConfig( system_type='pst-', # Downslope cut phi=30, # (counterclockwise positive) cut_length=300, ) pst_segments = [ Segment(length=5000, has_foundation=True, m=0), Segment(length=300, has_foundation=False, m=0), # Crack Segment ] # Scenario is Downslope PST with a 300mm cut Create SystemModel instance that combine...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[style] ~195-~195: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... is Downslope PST with a 300mm cut ``` Create SystemModel instance that combines the ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~196-~196: Use correct spacing
Context: ...m solving and field quantity extraction. python from weac.components import Config, ModelInput from weac.core.system_model import SystemModel # Example: build a model for the skier scenario defined above model_input = ModelInput( weak_layer=weak_layer, scenario_config=skier_config, layers=custom_layers, segments=skier_segments, ) system_config = Config( touchdown=True ) skier_system = SystemModel( model_input=model_input, config=system_config, ) Unknown constants are cached_properties;...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~218-~218: There might be a mistake here.
Context: ...em_config, ) ``` Unknown constants are cached_properties; calling `skier_system.unknown_constants...
(QB_NEW_EN_OTHER)
[grammar] ~218-~218: There might be a problem here.
Context: ...ar equations and extracts the constants. python C = skier_system.unknown_constants Analyzer handles rasterization + computation of ...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~224-~224: There might be a mistake here.
Context: ...weak-layer properties Sxx, Sxz, etc. Prepare the output by rasterizing the so...
(QB_NEW_EN)
[grammar] ~225-~225: Use correct spacing
Context: ...ates that are supported by a foundation. python from weac.analysis.analyzer import Analyzer skier_analyzer = Analyzer(skier_system) xsl_skier, z_skier, xwl_skier = skier_analyzer.rasterize_solution(mode="cracked") Gdif, GdifI, GdifII = skier_analyzer.differential_ERR() Ginc, GincI, GincII = skier_analyzer.incremental_ERR() # and Sxx, Sxz, Tzz, principal stress, incremental_potential, ... Visualize the results. ```python from w...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~237-~237: Use correct spacing
Context: ...tential, ... Visualize the results. python from weac.analysis.plotter import Plotter plotter = Plotter() # Visualize slab profile fig = plotter.plot_slab_profile( weak_layers=weak_layer, slabs=skier_system.slab, ) # Visualize deformations as a contour plot fig = plotter.plot_deformed( xsl_skier, xwl_skier, z_skier, skier_analyzer, scale=200, window=200, aspect=2, field="Sxx" ) # Plot slab displacements (using x-coordinates of all segments, xsl) plotter.plot_displacements(skier_analyzer, x=xsl_skier, z=z_skier) # Plot weak-layer stresses (using only x-coordinates of bedded segments, xwl) plotter.plot_stresses(skier_analyzer, x=xwl_skier, z=z_skier) ``` Compute output/field quantities for expo...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~260-~260: Use correct spacing
Context: ...ld quantities for exporting or plotting. python # Compute stresses in kPa in the weaklayer tau = skier_system.fq.tau(Z=z_skier, unit='kPa') sig = skier_system.fq.sig(Z=z_skier, unit='kPa') w = skier_system.fq.w(Z=z_skier, unit='um') # Example evaluation vertical displacement at top/mid/bottom of the slab u_top = skier_system.fq.u(Z=z_skier, h0=top, unit='um') u_mid = skier_system.fq.u(Z=z_skier, h0=mid, unit='um') u_bot = skier_system.fq.u(Z=z_skier, h0=bot, unit='um') psi = skier_system.fq.psi(Z=z_skier, unit='deg') ## Roadmap See the [open issues](https://g...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~276-~276: Use correct spacing
Context: ...='deg') ``` ## Roadmap See the [open issues](https://github.com...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~278-~278: Use correct spacing
Context: ...t of proposed features and known issues. ### v4.0 - [] Change to scenario & scenario...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~280-~280: Use correct spacing
Context: ...sed features and known issues. ### v4.0 - [] Change to scenario & scenario_config:...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~282-~282: Use correct spacing
Context: ...enario_config: InfEnd/Cut/Segment/Weight ### v3.2 <!-- - [ ] New mathematical foundat...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~284-~284: Use correct spacing
Context: ...fig: InfEnd/Cut/Segment/Weight ### v3.2 - [ ] Complex terrain through the addition...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~286-~286: There might be a mistake here.
Context: ...hrough the addition of out-of-plane tilt - [ ] Up, down, and cross-slope cracks ##...
(QB_NEW_EN)
[grammar] ~287-~287: Use correct spacing
Context: ...t - [ ] Up, down, and cross-slope cracks ### v3.1 - [ ] Improved CriteriaEvaluator O...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~289-~289: Use correct spacing
Context: ..., down, and cross-slope cracks ### v3.1 - [ ] Improved CriteriaEvaluator Optimizat...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~291-~291: Use correct spacing
Context: ...aluator Optimization (x2 time reduction) ## Release history ### v3.0 - Code Refact...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~293-~293: Use correct spacing
Context: ... (x2 time reduction) ## Release history ### v3.0 - Code Refactor - Input Validation...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~295-~295: Use correct spacing
Context: ...reduction) ## Release history ### v3.0 - Code Refactor - Input Validation - Modul...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~297-~297: There might be a mistake here.
Context: ...lease history ### v3.0 - Code Refactor - Input Validation - Modular + Object-Orie...
(QB_NEW_EN)
[grammar] ~298-~298: There might be a mistake here.
Context: ...v3.0 - Code Refactor - Input Validation - Modular + Object-Oriented ### v2.6 - F...
(QB_NEW_EN)
[grammar] ~299-~299: Use correct spacing
Context: ...t Validation - Modular + Object-Oriented ### v2.6 - Finite fracture mechanics implem...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~301-~301: Use correct spacing
Context: ...on - Modular + Object-Oriented ### v2.6 - Finite fracture mechanics implementation...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~305-~305: Use correct spacing
Context: ...tropic weak layer (?) - Add demo gif (?) ### v2.5 - Analyze slab touchdown in PST ex...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~307-~307: Use correct spacing
Context: ...k layer (?) - Add demo gif (?) ### v2.5 - Analyze slab touchdown in PST experiment...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~310-~310: There might be a mistake here.
Context: ...significantly improved API documentation ### v2.4 - Choose between slope-normal (`'-...
(QB_NEW_EN_OTHER)
[grammar] ~312-~312: Use correct spacing
Context: ...tly improved API documentation ### v2.4 - Choose between slope-normal ('-pst', `...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~314-~314: There might be a mistake here.
Context: ...st', 'vpst-'`) PST boundary conditions ### v2.3 - Stress plots on deformed contour...
(QB_NEW_EN_OTHER)
[grammar] ~316-~316: Use correct spacing
Context: ...st-'`) PST boundary conditions ### v2.3 - Stress plots on deformed contours - PSTs...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~318-~318: There might be a mistake here.
Context: ...s ### v2.3 - Stress plots on deformed contours - PSTs now account for slab touchdown ###...
(QB_NEW_EN_OTHER)
[grammar] ~319-~319: There might be a mistake here.
Context: ...rs - PSTs now account for slab touchdown ### v2.2 - Sign of inclination phi consis...
(QB_NEW_EN_OTHER)
[grammar] ~321-~321: Use correct spacing
Context: ...now account for slab touchdown ### v2.2 - Sign of inclination phi consistent wit...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~333-~333: There might be a mistake here.
Context: ...t use of coordinate system with downward pointing z-axis - Consitent top-to-botto...
(QB_NEW_EN_OTHER)
[grammar] ~333-~333: Ensure spelling is correct
Context: ...ate system with downward pointing z-axis - Consitent top-to-bottom numbering of slab layers ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~334-~334: There might be a mistake here.
Context: ...t top-to-bottom numbering of slab layers - Implementation of PSTs cut from either l...
(QB_NEW_EN)
[grammar] ~335-~335: There might be a mistake here.
Context: ...ayers - Implementation of PSTs cut from either left or right side ### v2.0 - Complet...
(QB_NEW_EN)
[grammar] ~335-~335: Use correct spacing
Context: ... PSTs cut from either left or right side ### v2.0 - Completely rewritten in 🐍 Pytho...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~337-~337: Use correct spacing
Context: ...from either left or right side ### v2.0 - Completely rewritten in 🐍 Python - Coup...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~362-~362: There might be a mistake here.
Context: ...ork the project 2. Initialize submodules bash git submodule update --init --recursive 3. Create your feature branch (`git checkou...
(QB_NEW_EN_OTHER)
[grammar] ~368-~368: There might be a mistake here.
Context: ...git checkout -b feature/amazingfeature) 4. Commit your changes (git commit -m 'Add...
(QB_NEW_EN_OTHER)
[style] ~369-~369: Consider using a more formal and expressive alternative to ‘amazing’.
Context: ...mazingfeature) 4. Commit your changes (git commit -m 'Add some amazing feature') 5. Push to the branch (git ...
(AWESOME)
[grammar] ~369-~369: There might be a mistake here.
Context: ...t commit -m 'Add some amazing feature') 5. Push to the branch (git push origin fea...
(QB_NEW_EN_OTHER)
[grammar] ~371-~371: There might be a mistake here.
Context: .../amazingfeature`) 6. Open a pull request ## Workflows [![Publish Python 🐍 releases ...
(QB_NEW_EN_OTHER)
🔇 Additional comments (7)
README.md (5)
122-130: Good: Runtime dependencies now align with pyproject/CI (Python ≥ 3.12).The dependency block looks correct and matches the refactor direction. Thanks for adding the surrounding blank lines.
230-234: Good: Analyzer now receives the correct model variable and typo fixed.This addresses prior feedback; the example should run as shown.
245-247: Good: Consistent variable names in Plotter usage.
weak_layers=weak_layerandslabs=skier_system.slabare now consistent with earlier definitions.
304-304: Good: “anisotropic” spelling corrected.Resolved prior typo.
314-314: Good: “vertical” spelling corrected.Resolved prior typo in PST boundary conditions section.
.github/workflows/tests.yml (2)
20-27: Nice use of setup-python with cachingUsing cache: 'pip' and check-latest is solid. This will speed up runs and keep 3.12 fresh.
33-34: Tests correctly fail the CI on test failuresConfirmed that
tests/run_tests.pyinvokes:if __name__ == "__main__": unittest_result = run_tests() sys.exit(0 if unittest_result.wasSuccessful() else 1)No changes required.
| jobs: | ||
| test: | ||
| name: Run unit tests 🤖 | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add a job timeout to prevent stuck runs consuming minutes
Guardrail to auto-cancel if tests hang.
Apply:
test:
name: Run unit tests 🤖
runs-on: ubuntu-latest
+ timeout-minutes: 20📝 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.
| jobs: | |
| test: | |
| name: Run unit tests 🤖 | |
| runs-on: ubuntu-latest | |
| jobs: | |
| test: | |
| name: Run unit tests 🤖 | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 20 |
🤖 Prompt for AI Agents
In .github/workflows/tests.yml around lines 13 to 16, the test job lacks a
timeout and can hang indefinitely; add a job timeout by including a
timeout-minutes field under the job definition (for example timeout-minutes: 20
or another project-appropriate value) to ensure GitHub Actions cancels stuck
runs automatically.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
.github/workflows/pylint.yml (3)
3-10: Good fix: quoted “on” key; triggers look correct.
Addresses the previous yamllint truthy warning and keeps the intended triggers.
30-68: Fix: lint targets don’t match repo reorg (weac_2/tests_2) and grep can fail the step.
- The PR moved code/tests to weac_2/tests_2. Current command still targets weac/ tests/ and will either do nothing useful or trigger a usage error (32).
- Several grep lines can return non-zero when there are zero matches or when the report file is empty/missing, causing the step to fail under bash -e.
Refactor to dynamically select existing targets and guard summaries.
- name: Run pylint analysis # Using repository pylint config (pyproject.toml) with comprehensive settings for scientific code run: | - exit_code=0 - python -m pylint --rcfile=pyproject.toml --output-format=parseable --output=pylint-report.txt weac/ tests/ || exit_code=$? + set +e # we'll manage exit codes manually + exit_code=0 + : > pylint-report.txt # ensure file exists even if pylint errors early + + # Build target list compatible with repo reorg (prefer new dirs, fall back to legacy) + TARGET_DIRS="" + for d in weac_2 weac tests_2 tests; do + [ -d "$d" ] && TARGET_DIRS="$TARGET_DIRS $d" + done + if [ -z "$TARGET_DIRS" ]; then + echo "No target directories found for pylint (checked: weac_2, weac, tests_2, tests)." + echo "Skipping pylint run." + exit 0 + fi + + python -m pylint --output-format=parseable --output=pylint-report.txt $TARGET_DIRS || exit_code=$? echo "Pylint finished with exit code $exit_code." echo echo "Pylint exit code meaning:" if [ $exit_code -eq 0 ]; then echo "-> No issues found"; fi if [ $((exit_code & 1)) -ne 0 ]; then echo "-> Fatal message issued"; fi if [ $((exit_code & 2)) -ne 0 ]; then echo "-> Error message issued"; fi if [ $((exit_code & 4)) -ne 0 ]; then echo "-> Warning message issued"; fi if [ $((exit_code & 8)) -ne 0 ]; then echo "-> Refactor message issued"; fi if [ $((exit_code & 16)) -ne 0 ]; then echo "-> Convention message issued"; fi if [ $((exit_code & 32)) -ne 0 ]; then echo "-> Usage error"; fi echo echo 'Error type counts:' - grep -oP '[A-Z]\d+\([a-z\-]+\)' pylint-report.txt | sort | uniq -c | sort -nr + grep -oP '[A-Z]\d+\([a-z\-]+\)' pylint-report.txt | sort | uniq -c | sort -nr || true echo echo 'Errors per file:' - grep -oP '^[\w\-\/]+\.py' pylint-report.txt | sort | uniq -c | sort -nr + grep -oP '^[\w\-\/]+\.py' pylint-report.txt | sort | uniq -c | sort -nr || true echo echo 'Total errors:' - grep -oP '^[\w\-\/]+\.py' pylint-report.txt | wc -l + grep -oP '^[\w\-\/]+\.py' pylint-report.txt | wc -l || true echo grep 'Your code' pylint-report.txt || true # Fail on fatal, error, and usage error. # These are severe and should block PRs. # Warnings (4), refactors (8), and conventions (16) will not cause a failure. fail_on_codes=$((1 | 2 | 32)) if [ $((exit_code & fail_on_codes)) -ne 0 ]; then echo "Failing CI due to fatal/error/usage messages from pylint." exit 1 else echo "Pylint check passed. No fatal/error/usage messages." fiNotes:
- If your pylint config lives in pyproject.toml, you can drop the explicit --rcfile flag; pylint auto-detects pyproject. Keep it only if you’ve verified it’s needed.
12-15: Add least-privilege permissions to harden the workflow.
Explicitly scope the token to read-only. Top-level preferred so all jobs inherit it."on": pull_request: types: [opened, reopened, synchronize, ready_for_review] workflow_call: workflow_dispatch: +permissions: + contents: read + jobs: pylint:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/pylint.yml(1 hunks)
| - name: Set up Python 3.12 | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.12" | ||
| cache: "pip" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: improve pip cache effectiveness with dependency path hints.
actions/setup-python can hash your requirements to invalidate cache correctly.
- name: Set up Python 3.12
uses: actions/setup-python@v5
with:
python-version: "3.12"
cache: "pip"
+ cache-dependency-path: |
+ requirements.txt
+ pyproject.toml
+ setup.cfg
+ setup.py
+ environment.yml📝 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.
| - name: Set up Python 3.12 | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: "3.12" | |
| cache: "pip" | |
| - name: Set up Python 3.12 | |
| uses: actions/setup-python@v5 | |
| with: | |
| python-version: "3.12" | |
| cache: "pip" | |
| cache-dependency-path: | | |
| requirements.txt | |
| pyproject.toml | |
| setup.cfg | |
| setup.py | |
| environment.yml |
🤖 Prompt for AI Agents
In .github/workflows/pylint.yml around lines 19 to 24, the workflow uses
actions/setup-python with cache: "pip" but does not provide
cache-dependency-path, so the cache won’t be invalidated when requirements
change; add the cache-dependency-path input pointing to your dependency files
(e.g. **/requirements.txt, **/pyproject.toml, **/setup.cfg or other files you
use) so the action will hash those files and invalidate the pip cache when they
change.
| echo 'Error type counts:' | ||
| grep -oP '[A-Z]\d+\([a-z\-]+\)' pylint-report.txt | sort | uniq -c | sort -nr | ||
| echo | ||
| echo 'Errors per file:' | ||
| grep -oP '^[\w\-\/]+\.py' pylint-report.txt | sort | uniq -c | sort -nr | ||
| echo | ||
| echo 'Total errors:' | ||
| grep -oP '^[\w\-\/]+\.py' pylint-report.txt | wc -l | ||
| echo | ||
| grep 'Your code' pylint-report.txt || true |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: drop grep -P for portability.
GNU grep -P is fine on ubuntu-latest, but can be disabled elsewhere. Consider awk/sed for portability if you reuse locally.
Example replacement for the three grep -oP pipelines:
- Error types:
awk '{ if (match($0, /[A-Z][0-9]+([a-z-]+)/, a)) print a[0] }' pylint-report.txt | sort | uniq -c | sort -nr || true - Per-file:
awk -F: 'NF>1 &&$1 ~ /.py$ / {print $1}' pylint-report.txt | sort | uniq -c | sort -nr || true - Total errors:
awk -F: 'NF>1 &&$1 ~ /.py$ /' pylint-report.txt | wc -l || true
🤖 Prompt for AI Agents
In .github/workflows/pylint.yml around lines 47 to 56 the workflow uses grep -P
(Perl regex) which is not portable; replace the three grep -oP pipelines with
POSIX-safe awk (or sed) equivalents that extract the same fields (error type
tokens, filenames, and count of errors), preserve the same sorting/uniq logic
and ensure the pipeline exits zero on no matches (append || true where needed),
and keep the surrounding echo lines and output ordering unchanged.
🚨 Bugbot Trial ExpiredYour Bugbot trial has expired. Please purchase a license in the Cursor dashboard to continue using Bugbot. |
…ecated highlighting extension
Complete Refactor of CodeBase
Reasons:
Draft:
In the draft the weac package is refactored in weac_2 and integration tests are provided in tests_2.
Additionally, a streamlit App is provided to test the implementation.
Next steps:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores