Feat/Split WL and Layer + Feat/Add Schöttner E(ρ) elasticity + Chore/Test Suite to Pytest - #54
Conversation
- Extracted `WeakLayer` into `weac.components.weak_layer`, leaving slab layers in `layer`. - Replaced the weak-layer Young's modulus with Schottner's density-dependent formulation and matching constants. - Updated imports, docs, and tests for the split and new elasticity law.
- Replaced unittest with pytest in all test files for consistency and improved functionality. - Added pytest configuration in pyproject.toml and updated test paths. - Removed the legacy test runner script and adjusted test execution commands in CI workflows. - Enhanced existing tests to utilize pytest assertions for better readability and maintainability.
Keep Schöttner E(ρ) within its intended ≤250 kg/m³ range for the named presets. Co-authored-by: Cursor <cursoragent@cursor.com>
Bring in lowered weak-layer preset densities (100/150/200 kg/m³). Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 44 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe change separates ChangesWeakLayer separation and model updates
Pytest migration and project support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/weac/components/weak_layer.py`:
- Around line 129-133: Update WeakLayer.model_post_init to enforce the
schottner_fc_dh density limit before deriving E: when E is zero or unset, reject
rho values above 250 with a validation error, while allowing rho=250 and
preserving caller-supplied E or other methods. Add boundary tests covering
rho=250 and rho=251.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 15e44f5f-fa5e-4bab-91cc-465a34b28c9b
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
.github/workflows/tests.yml.gitignoreREADME.mdTODO.mddocs/sphinx/weac.components.rstdocs/sphinx/weac.components.weak_layer.rstpyproject.tomlsrc/weac/analysis/plotter.pysrc/weac/components/__init__.pysrc/weac/components/layer.pysrc/weac/components/model_input.pysrc/weac/components/presets.pysrc/weac/components/weak_layer.pysrc/weac/constants.pysrc/weac/core/slab_touchdown.pytests/analysis/test_analyzer.pytests/analysis/test_criteria_evaluator.pytests/analysis/test_slab_tensile_comparisons.pytests/components/test_configs.pytests/components/test_layer.pytests/conftest.pytests/core/test_eigensystem.pytests/core/test_field_quantities.pytests/core/test_scenario.pytests/core/test_slab.pytests/core/test_slab_touchdown.pytests/core/test_system_model.pytests/run_tests.pytests/test_comparison_results.pytests/test_regression_simulation.pytests/utils/test_json_helpers.pytests/utils/test_misc.pytests/utils/test_snowpilot_parser.py
💤 Files with no reviewable changes (1)
- tests/run_tests.py
There was a problem hiding this comment.
Pull request overview
This PR refactors WEAC’s component model boundaries by extracting WeakLayer into its own module, introduces a Schöttner FC&DH density-based elasticity option for the weak layer, and migrates the Python test suite + CI execution from unittest to pytest.
Changes:
- Split weak-layer modeling into
weac.components.weak_layerwhile keeping slab layers inLayer, updating imports/presets/defaults accordingly. - Add Schöttner FC&DH density-law Young’s modulus support (
E_method="schottner_fc_dh") and adjust weak-layer preset densities to stay within the stated applicability range. - Convert the full test suite + CI runner from
unittest(custom runner) topytest(fixtures/parametrization,pyproject.toml+ workflow updates).
Reviewed changes
Copilot reviewed 32 out of 34 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Adds pytest and its dependencies to the locked environment. |
| TODO.md | Updates roadmap items; removes now-completed “Swap to Pytest” note and adds new modeling TODOs. |
| tests/utils/test_snowpilot_parser.py | Migrates SnowPilot parser tests from unittest to pytest fixtures/asserts. |
| tests/utils/test_misc.py | Migrates misc utility tests to pytest and updates skip behavior. |
| tests/utils/test_json_helpers.py | Converts JSON helper tests to pytest; adds parametrized cases. |
| tests/test_regression_simulation.py | Migrates regression suite to pytest and updates numeric baselines. |
| tests/test_comparison_results.py | Migrates integration comparison tests to pytest assertions/approximations. |
| tests/run_tests.py | Removes legacy unittest-based test runner script. |
| tests/core/test_system_model.py | Refactors SystemModel tests to pytest fixtures and helper builder. |
| tests/core/test_slab.py | Migrates slab tests to pytest and approx-based float checks. |
| tests/core/test_slab_touchdown.py | Converts touchdown tests to pytest fixtures and assertions. |
| tests/core/test_scenario.py | Migrates Scenario tests to pytest fixtures and pytest.raises. |
| tests/core/test_field_quantities.py | Converts extensive FieldQuantities suite to pytest fixtures. |
| tests/core/test_eigensystem.py | Migrates eigensystem tests to pytest fixtures and assertions. |
| tests/conftest.py | Adds pytest-wide logging setup. |
| tests/components/test_layer.py | Updates layer/weak-layer tests for the split + Schöttner modulus; migrates to pytest. |
| tests/components/test_configs.py | Migrates config/scenario/criteria/segment/model-input tests to pytest. |
| tests/analysis/test_slab_tensile_comparisons.py | Uses pytest fixture + parametrization for ordering comparisons. |
| tests/analysis/test_analyzer.py | Migrates Analyzer tests to pytest fixtures and assertions. |
| src/weac/core/slab_touchdown.py | Updates WeakLayer import to new module path. |
| src/weac/constants.py | Introduces Schöttner-law constants (CS0, CS1). |
| src/weac/components/weak_layer.py | New WeakLayer model with Schöttner FC&DH elasticity method + stiffness derivations. |
| src/weac/components/presets.py | Updates WeakLayer presets (densities) to align with Schöttner range and derived E. |
| src/weac/components/model_input.py | Updates WeakLayer import and default weak-layer factory (now density-law derived). |
| src/weac/components/layer.py | Removes embedded WeakLayer implementation and re-exports WeakLayer for compatibility. |
| src/weac/components/init.py | Updates exports to import WeakLayer from the new module. |
| src/weac/analysis/plotter.py | Updates WeakLayer import to new module path. |
| README.md | Updates test-running docs to use uv run pytest; adds v3.2 release notes for the refactor. |
| pyproject.toml | Adds pytest dev dependency and pytest config (testpaths). |
| docs/sphinx/weac.components.weak_layer.rst | Adds Sphinx automodule docs for the new weak-layer module. |
| docs/sphinx/weac.components.rst | Includes the new weak-layer module in the components documentation. |
| .gitignore | Adds todo/ directory to ignored local-dev artifacts. |
| .github/workflows/tests.yml | Updates CI to install dev extras and run pytest directly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Quality Assessment — PR #54
Reviewed the diff for WeakLayer module split, Schöttner FC&DH elasticity (E_method="schottner_fc_dh"), preset/default changes, and pytest migration.
Outcome: 1 medium-severity finding. No high/critical bugs or security vulnerabilities identified.
Validated areas
- Schöttner formula constants and unit tests match paper benchmarks (~1.5 MPa @ 150 kg/m³, ~24 MPa @ 250 kg/m³)
WeakLayermodule split + lazy re-export fromlayer.pyworks at runtime- Preset density/E derivation tests pass; 232/234 tests pass locally (2
test_comparison_resultsfailures are reference-env setup, not PR logic; CI is green) collapse_height >= hvalidation raises as expected- No injection/auth/network attack surface in modified code
Top finding
- Schöttner density extrapolation (Medium): Default
schottner_fc_dhsilently extrapolates beyond the documented ≤250 kg/m³ range, producing materially wrongE/kn/kt(e.g. ρ=300 → E≈65 MPa)
Prior threads
- No prior automation review threads from this workflow to reconcile.
- CodeRabbit/Copilot threads on this PR are outside this automation's scope.
Sent by Cursor Automation: Check PR
|
|
||
| model_config = ConfigDict( | ||
| frozen=True, | ||
| extra="forbid", |
There was a problem hiding this comment.
Category: Bug / Test Coverage Gap
Severity: Medium
Issue & Impact: With the new default E_method="schottner_fc_dh", E is derived from density without enforcing the documented ≤250 kg/m³ calibration range (v3.2 release notes). Callers that omit E can silently extrapolate outside the validated window — e.g. WeakLayer(rho=300, h=10) yields E≈65 MPa (vs ~24 MPa at ρ=250), which also skews derived G, kn, and kt. The same path is reachable via weak_layer_from_preset(..., rho=300).
Remediation: When E_method == "schottner_fc_dh" and E is unset/zero, reject rho > 250 with a ValueError (allow ρ=250), or require an explicit E / alternate E_method. Add boundary tests for rho=250 (accepted) and rho=251 (rejected).
Summary
WeakLayerintoweac.components.weak_layer, keeping slab layers inLayerE_method="schottner_fc_dh")pyproject.toml)Summary by CodeRabbit
New Features
Documentation
Chores