Skip to content

Conversation

@pillowbeast
Copy link
Collaborator

@pillowbeast pillowbeast commented Nov 24, 2025

Evaluate SSERR Modification to always use 0.0 degrees in line with initial implementation.

Changed dev & CI environment to uv for modern standard.

Summary by CodeRabbit

  • Chores

    • Modernized CI to a unified uv-based flow and updated workflow actions
    • Introduced an optional build dependency group and reorganized build tooling
    • Added a Python runtime spec file and adjusted ignore rules
  • Refactor

    • Modernized public type annotations to use built-in generics and union syntax
  • Tests

    • Enabled touchdown behavior in tests and updated regression baselines
  • Documentation

    • Added a Development Setup guide with uv-based instructions

✏️ Tip: You can customize this high-level summary in your review settings.

Copilot AI review requested due to automatic review settings November 24, 2025 12:35
@pillowbeast pillowbeast added the bug Something isn't working label Nov 24, 2025
@coderabbitai
Copy link

coderabbitai bot commented Nov 24, 2025

Warning

Rate limit exceeded

@pillowbeast has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 8 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 780b5c0 and 1bfcb68.

📒 Files selected for processing (5)
  • README.md (2 hunks)
  • pyproject.toml (1 hunks)
  • src/weac/analysis/criteria_evaluator.py (11 hunks)
  • src/weac/analysis/plotter.py (11 hunks)
  • src/weac/core/system_model.py (6 hunks)

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

CI workflows migrated from Python/pip to uv tooling (astral-sh/setup-uv); project docs and README updated with uv development steps; a .python-version file was added and its ignore-rule removed; many modules switched typing from typing generics to built-in list[...]/PEP 604 unions; tests enabled touchdown and updated regression baselines.

Changes

Cohort / File(s) Summary
GitHub Actions Workflows Migration
​.github/workflows/docs.yml, ​.github/workflows/format.yml, ​.github/workflows/pylint.yml, ​.github/workflows/release.yml, ​.github/workflows/tests.yml
Updated actions/checkout v4→v6; replaced Python setup steps with astral-sh/setup-uv@v7 (version: latest); replaced pip/python commands with uv sync --extra ... and uv run ... invocations for install, build, lint, test, and docs while preserving step ordering/intent.
Project metadata & packaging
pyproject.toml
Added optional dependency group build = ["build", "twine"], moving build tooling out of dev extras.
Docs & README
README.md
Added "Development Setup" section with uv-based developer commands and reordered table of contents to include it.
Version file & ignore rules
​.python-version, ​.gitignore
Added .python-version containing 3.12; removed .python-version entry from .gitignore.
Typing modernization — analysis & core modules
src/weac/analysis/criteria_evaluator.py, src/weac/analysis/plotter.py, src/weac/core/eigensystem.py, src/weac/core/scenario.py, src/weac/core/slab.py, src/weac/core/slab_touchdown.py, src/weac/core/system_model.py, src/weac/core/unknown_constants_solver.py
Replaced typing generics (List, Tuple, Optional, Union, NDArray) with built-in generics (list[...], tuple[...]) and PEP 604 union syntax (`T
Typing modernization — utilities & components
src/weac/components/model_input.py, src/weac/utils/snowpilot_parser.py, src/weac/logging_config.py
Updated dataclass/func annotations from List/Dict/Tuple/Optional to built-in generics and union syntax; removed now-unused typing imports; small doc/comment edits.
Tests — touchdown & baselines
tests/analysis/test_criteria_evaluator.py, tests/test_regression_simulation.py
Tests now construct SystemModel/Config with touchdown=True; regression expected values updated (touchdown_distance and SSERR baselines/tolerances adjusted).
Test helpers typing
tests/utils/weac_reference_runner.py
Updated test helper type hints to built-in generics and PEP 604 unions (e.g., dict[...], `ReferenceEnv

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Runner as GitHub Runner
    participant Checkout as actions/checkout (v6)
    participant SetupUV as astral-sh/setup-uv (uv)
    participant UVCLI as uv (uv sync / uv run / uv build)
    participant Steps as CI Steps

    Runner->>Checkout: checkout code
    Runner->>SetupUV: install/setup uv
    Note right of SetupUV: runtime/tooling provided by uv
    Runner->>UVCLI: uv sync --extra <dev|docs|build>
    UVCLI-->>Steps: dependencies available
    Runner->>UVCLI: uv run <tool> (ruff/pylint/sphinx-apidoc/python tests)
    alt publish
      Runner->>UVCLI: uv build
      Runner->>UVCLI: uv run twine upload dist/*
    end
    Steps-->>Runner: results (success/failure)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Areas needing focused review:
    • src/weac/core/system_model.py (signature changes, cached_property types, potential behavioral edge cases).
    • src/weac/analysis/criteria_evaluator.py (SSERR/slab-touchdown logic and system_copy adjustments).
    • CI workflow files (.github/workflows/*) to confirm uv sync/run semantics match previous environment and dependency resolution.
    • tests/test_regression_simulation.py to validate updated numeric baselines and tolerances.

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Bug fix/sserr' is vague and generic, using non-descriptive slash notation that doesn't clearly convey what the specific bug fix addresses or what SSERR change was made. Consider a more descriptive title like 'Fix SSERR evaluation to use 0.0 degrees phi' or 'Correct SSERR calculation for touchdown scenarios' that explains the actual bug being fixed.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 81.82% which is sufficient. The required threshold is 80.00%.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot finished reviewing on behalf of pillowbeast November 24, 2025 12:37
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug in the SSERR (Steady State Energy Release Rate) evaluation by enforcing the use of 0.0 degrees for the phi parameter, aligning with the initial implementation requirement that slab touchdown only works with a flat slab. The PR also modernizes the development and CI environment by migrating from pip to uv for package management.

  • Fixed SSERR evaluation to always use phi=0.0 degrees and dynamic segment lengths based on l_BC
  • Migrated all CI workflows and development setup to use uv for faster package management
  • Updated typing annotations to use modern Python 3.9+ syntax (list instead of List)

Reviewed changes

Copilot reviewed 10 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/weac/analysis/criteria_evaluator.py Fixed SSERR evaluation to use phi=0.0 and l_BC-based segments; modernized type hints from List to list
tests/test_regression_simulation.py Updated test to use touchdown=True and adjusted baseline values for new SSERR calculations
tests/analysis/test_criteria_evaluator.py Updated test to use touchdown=True for SSERR evaluation
README.md Added comprehensive Development Setup section documenting uv usage
.python-version Added Python 3.12 version specification for uv
.gitignore Removed .python-version exclusion to track it in repository
.github/workflows/*.yml Migrated all workflows from pip to uv and updated action versions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
src/weac/analysis/criteria_evaluator.py (1)

675-675: Critical: AttributeError when touchdown is disabled on input system.

Line 675 accesses system.slab_touchdown.l_BC before ensuring touchdown is enabled. According to the system_model.py code, the slab_touchdown property returns None when config.touchdown is False, which will cause an AttributeError when accessing .l_BC. The touchdown configuration is only enabled on the copy at line 686, which occurs after this access.

Apply this diff to fix the issue by enabling touchdown before accessing its properties:

 system_copy = copy.deepcopy(system)
+system_copy.config.touchdown = True
+# Need to trigger slab_touchdown calculation with a temporary scenario
+segments_temp = [
+    Segment(length=5e3, has_foundation=True, m=0.0),
+    Segment(length=system.scenario.cut_length if system.scenario.cut_length > 0 else 1000, has_foundation=False, m=0.0),
+]
+scenario_config_temp = ScenarioConfig(
+    system_type="vpst-" if vertical else "pst-",
+    phi=0.0,
+    cut_length=system.scenario.cut_length if system.scenario.cut_length > 0 else 1000,
+)
+system_copy.update_scenario(segments=segments_temp, scenario_config=scenario_config_temp)
+l_BC = system_copy.slab_touchdown.l_BC
-l_BC = system.slab_touchdown.l_BC
-
 segments = [
     Segment(length=5e3, has_foundation=True, m=0.0),
     Segment(length=2 * l_BC, has_foundation=False, m=0.0),
 ]
 scenario_config = ScenarioConfig(
     system_type="vpst-" if vertical else "pst-",
     phi=0.0,  # Slab Touchdown works only for flat slab
     cut_length=2 * l_BC,
 )
-system_copy.config.touchdown = True
 system_copy.update_scenario(segments=segments, scenario_config=scenario_config)
🧹 Nitpick comments (2)
README.md (1)

162-162: Clarify the uv sync workflow for developers.

The instructions show two separate uv sync invocations: once at line 162 (basic) and again at line 168 (with --extra dev). This could be clearer—consider explicitly stating whether developers should run both commands in sequence, or if only the dev version is needed for local development. Currently, it's ambiguous whether running just line 162 is sufficient for basic usage.

Consider restructuring this section to be more explicit:

# For basic setup (if only running the package):
uv sync

# For development (recommended for contributors):
uv sync --extra dev

Or, if the intent is sequential setup, clarify: "After initial setup, install development dependencies..."

Also applies to: 168-168

.github/workflows/release.yml (1)

22-22: Consider using uv sync --extra build for consistency.

Using uv pip install build twine bypasses the project's dependency management defined in pyproject.toml. For consistency with other workflows, consider defining a [project.optional-dependencies.build] group and using uv sync --extra build instead.

If you add a build extras group to pyproject.toml:

[project.optional-dependencies]
build = ["build", "twine"]

Then apply this diff:

-        run: uv pip install build twine
+        run: uv sync --extra build
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 0e7ae3f and b136711.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (11)
  • .github/workflows/docs.yml (1 hunks)
  • .github/workflows/format.yml (1 hunks)
  • .github/workflows/pylint.yml (1 hunks)
  • .github/workflows/release.yml (1 hunks)
  • .github/workflows/tests.yml (1 hunks)
  • .gitignore (0 hunks)
  • .python-version (1 hunks)
  • README.md (2 hunks)
  • src/weac/analysis/criteria_evaluator.py (10 hunks)
  • tests/analysis/test_criteria_evaluator.py (1 hunks)
  • tests/test_regression_simulation.py (2 hunks)
💤 Files with no reviewable changes (1)
  • .gitignore
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-13T08:08:46.186Z
Learnt from: pillowbeast
Repo: 2phi/weac PR: 8
File: pyproject.toml:39-39
Timestamp: 2025-08-13T08:08:46.186Z
Learning: For the weac project in pyproject.toml, the user prefers to exclude CITATION.cff and image files from package data, keeping only core package files in the built wheels.

Applied to files:

  • README.md
🧬 Code graph analysis (3)
tests/analysis/test_criteria_evaluator.py (1)
src/weac/components/config.py (1)
  • Config (16-28)
src/weac/analysis/criteria_evaluator.py (3)
src/weac/components/segment.py (1)
  • Segment (8-31)
src/weac/core/system_model.py (1)
  • slab_touchdown (158-195)
src/weac/components/scenario_config.py (1)
  • ScenarioConfig (15-72)
tests/test_regression_simulation.py (2)
src/weac/core/system_model.py (1)
  • SystemModel (38-413)
src/weac/components/config.py (1)
  • Config (16-28)
🔇 Additional comments (17)
README.md (3)

71-77: Table of contents properly updated.

The renumbering correctly inserts the new Development Setup section and maintains consistent ordering for subsequent sections.


133-169: Development Setup section is well-structured and comprehensive.

The documentation clearly guides developers through uv installation, environment setup, and basic workflows with appropriate OS-specific instructions and progressive complexity. The progression from base installation → dev dependencies → testing/linting is logical and intuitive.


207-207: No changes needed—syntax is correct.

The uv pip install -e . command is the correct uv syntax for installing the current project in editable mode, as documented in the official uv documentation.

.python-version (1)

1-1: LGTM! Python version specification for uv.

The .python-version file correctly specifies Python 3.12 for uv-based tooling, aligning with the migration across all workflow files.

.github/workflows/tests.yml (1)

18-29: LGTM! Clean migration to uv tooling.

The workflow has been successfully migrated to use uv for dependency management and test execution. The changes are consistent with modern Python tooling practices.

.github/workflows/format.yml (1)

17-30: LGTM! Formatting workflow migrated to uv.

The migration to uv is correctly implemented with appropriate use of --extra dev for development dependencies.

.github/workflows/pylint.yml (1)

17-31: LGTM! Static analysis workflow migrated to uv.

The PyLint workflow has been properly migrated to use uv while preserving the comprehensive static analysis configuration.

.github/workflows/docs.yml (2)

30-48: Documentation workflow migrated to uv.

The migration is correct, though please verify the dependency group configuration as noted in the separate comment.


39-40: No changes needed — the 'docs' dependency group is properly configured.

Verification confirms the docs extra exists in pyproject.toml with sphinx and sphinxawesome-theme. The workflow command uv sync --extra docs will execute successfully.

.github/workflows/release.yml (1)

15-31: Release workflow migrated to uv.

The migration correctly implements uv-based package building and publishing. Consider the dependency management suggestion in the separate comment for improved consistency.

tests/analysis/test_criteria_evaluator.py (1)

199-199: LGTM! Touchdown explicitly enabled for SSERR test.

The change from config=self.config to config=Config(touchdown=True) explicitly enables touchdown mode for SSERR evaluation, aligning with the PR's objective to standardize SSERR behavior. This makes the test's intent clearer and ensures touchdown is enabled regardless of self.config state.

tests/test_regression_simulation.py (2)

406-406: LGTM! Touchdown mode enabled for criteria evaluator tests.

The change from Config(touchdown=False) to Config(touchdown=True) aligns with the PR's objective to standardize SSERR behavior with touchdown enabled. This ensures regression tests validate the intended production configuration.


424-425: The review comment's baseline change claims are accurate; verify tolerance relaxation is intentional.

The git diff confirms all stated changes:

  • Config(touchdown=False)Config(touchdown=True) on line 406
  • touchdown_distance: 1320.108936137 → 1265.55192469 (−4.13%)
  • SSERR: 2.168112101045914 → 2.123992 (−2.06%)
  • Tolerance: rtol=1e-8rtol=1e-6 (100x relaxation)

The configuration change to touchdown=True justifies the baseline value updates. However, the tolerance relaxation warrants confirmation: while 1e-6 remains stricter than other test assertions (which use rtol=1e-2), ensure this relaxation is intentional and reflects the expected precision limits of the touchdown-enabled calculation path rather than masking precision degradation.

src/weac/analysis/criteria_evaluator.py (4)

12-12: LGTM: Clean migration to built-in type annotations.

The removal of List from typing imports aligns with modern Python 3.9+ practices, where built-in generic types are preferred.


37-42: LGTM: Consistent type annotation modernization.

The dataclass field annotations have been correctly updated to use built-in list types, improving consistency with modern Python standards.

Also applies to: 133-133, 135-135, 147-148


683-683: LGTM: Correct implementation of SSERR fix.

Setting phi=0.0 explicitly aligns with the PR objective to fix "SSERR behavior to always use 0.0 degrees." The comment clearly explains the constraint.


830-830: LGTM: Method signatures modernized consistently.

Return type annotations and docstrings have been correctly updated to use built-in list types, maintaining consistency throughout the codebase.

Also applies to: 843-843, 933-933, 949-949, 1091-1091

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
pyproject.toml (1)

49-49: Good dependency reorganization; clarify the comment.

The separation of build tools (build and twine) into a dedicated optional-dependencies group is clean and aligns well with the uv migration. This allows developers to install only what they need. However, the comment on line 79 is slightly ambiguous—it refers only to "build" being moved but both "build" and "twine" were removed from dev.

Consider making the comment more explicit:

-    # build moved to build optional dependencies
+    # build and twine moved to build optional dependencies

Also applies to: 78-79

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b136711 and 80224eb.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (4)
  • .github/workflows/release.yml (1 hunks)
  • pyproject.toml (2 hunks)
  • src/weac/analysis/criteria_evaluator.py (10 hunks)
  • tests/test_regression_simulation.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/test_regression_simulation.py
  • src/weac/analysis/criteria_evaluator.py
🔇 Additional comments (4)
.github/workflows/release.yml (4)

24-25: Build command looks good.

The switch to uv build is the correct approach for uv-based projects and aligns well with the tooling migration.


27-31: Code is correct as-is.

The verification confirms twine is included in the project dependencies (build = ["build", "twine"]), so the uv run twine upload command in the workflow will have access to it. No changes needed.


21-22: ****

The build extra is properly defined in pyproject.toml as build = ["build", "twine"]. The command uv sync --extra build is valid and will execute correctly.


15-15: No compatibility issues detected with checkout@v6 upgrade.

This workflow uses GitHub-hosted runners (macos-latest), which are automatically maintained at current versions meeting v6 requirements. The breaking changes between v4 and v6—particularly the credential storage change and runner version requirement—do not impact this workflow because:

  1. Credentials are passed via environment variables (TWINE_USERNAME, TWINE_PASSWORD), not git config
  2. No subsequent git operations depend on persisted credentials
  3. GitHub-hosted runners meet the v2.329.0+ minimum requirement

The upgrade to actions/checkout@v6 is safe and requires no changes.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
src/weac/analysis/criteria_evaluator.py (1)

674-687: Critical bug: accessing slab_touchdown on wrong system.

Line 676 accesses system.slab_touchdown.l_BC (the original system), but touchdown was enabled on system_copy at line 675. This will cause an AttributeError if the original system doesn't have touchdown enabled.

Additionally, there's a sequence issue:

  1. Line 674 calls update_scenario with phi=0.0 but touchdown is not yet enabled in the config
  2. Line 675 sets touchdown = True on the config, but doesn't trigger re-initialization of slab_touchdown
  3. Line 676 attempts to access l_BC which may not exist

Apply this diff to fix the issue:

     system_copy = copy.deepcopy(system)
-    system_copy.update_scenario(scenario_config=ScenarioConfig(phi=0.0))
-    system_copy.config.touchdown = True
-    l_BC = system.slab_touchdown.l_BC
+    system_copy.config.touchdown = True
+    system_copy.update_scenario(scenario_config=ScenarioConfig(phi=0.0))
+    l_BC = system_copy.slab_touchdown.l_BC

This ensures:

  1. Touchdown is enabled before calling update_scenario, allowing proper initialization
  2. The correct system (system_copy) is used to access slab_touchdown
🧹 Nitpick comments (2)
src/weac/core/system_model.py (1)

365-373: Align z() runtime sequence check with its type hint (optional)

z is annotated with x: float | Sequence[float] | np.ndarray, but the runtime branch checks isinstance(x, (list, tuple, np.ndarray)). If you ever want to accept other Sequence implementations (e.g. array.array, custom sequence types), you could slightly generalize the check:

-        if isinstance(x, (list, tuple, np.ndarray)):
+        if isinstance(x, (np.ndarray, Sequence)):

This keeps current behavior and better matches the annotation, while still treating scalars as the else-branch.

Also applies to: 398-411

src/weac/analysis/plotter.py (1)

250-257: plot_slab_profile union types match runtime behavior; consider enforcing length consistency (optional)

Typing weak_layers / slabs as list[WeakLayer] | WeakLayer and list[Slab] | Slab plus labels: list[str] | str | None mirrors the runtime logic that normalizes scalars to 1-element lists and handles string vs list labels.

One optional robustness tweak: currently there’s no explicit check that len(weak_layers) == len(slabs), and a mismatch could raise an IndexError in the max_height loop. Adding a length check early would give a clearer error:

-        if isinstance(weak_layers, WeakLayer):
+        if isinstance(weak_layers, WeakLayer):
             weak_layers = [weak_layers]
         if isinstance(slabs, Slab):
             slabs = [slabs]
+
+        if len(weak_layers) != len(slabs):
+            raise ValueError("Number of weak_layers must match number of slabs")

Purely defensive; otherwise the annotations and behavior are aligned.

Also applies to: 263-273, 279-295

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 80224eb and a1d402c.

📒 Files selected for processing (12)
  • src/weac/analysis/criteria_evaluator.py (11 hunks)
  • src/weac/analysis/plotter.py (10 hunks)
  • src/weac/components/model_input.py (1 hunks)
  • src/weac/core/eigensystem.py (6 hunks)
  • src/weac/core/scenario.py (4 hunks)
  • src/weac/core/slab.py (2 hunks)
  • src/weac/core/slab_touchdown.py (3 hunks)
  • src/weac/core/system_model.py (5 hunks)
  • src/weac/core/unknown_constants_solver.py (4 hunks)
  • src/weac/logging_config.py (1 hunks)
  • src/weac/utils/snowpilot_parser.py (3 hunks)
  • tests/utils/weac_reference_runner.py (4 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/weac/core/slab.py
🧰 Additional context used
🧬 Code graph analysis (5)
src/weac/components/model_input.py (2)
src/weac/components/layer.py (1)
  • Layer (97-171)
src/weac/components/segment.py (1)
  • Segment (8-31)
src/weac/analysis/plotter.py (2)
src/weac/core/system_model.py (1)
  • SystemModel (37-412)
src/weac/core/slab.py (1)
  • Slab (11-147)
src/weac/core/system_model.py (4)
src/weac/components/model_input.py (1)
  • ModelInput (27-75)
src/weac/components/config.py (1)
  • Config (16-28)
src/weac/components/segment.py (1)
  • Segment (8-31)
src/weac/components/scenario_config.py (1)
  • ScenarioConfig (15-72)
src/weac/analysis/criteria_evaluator.py (4)
src/weac/components/segment.py (1)
  • Segment (8-31)
src/weac/core/field_quantities.py (1)
  • tau (105-114)
src/weac/core/system_model.py (2)
  • update_scenario (318-340)
  • slab_touchdown (157-194)
src/weac/components/scenario_config.py (1)
  • ScenarioConfig (15-72)
src/weac/utils/snowpilot_parser.py (1)
src/weac/components/layer.py (2)
  • Layer (97-171)
  • WeakLayer (174-276)
🔇 Additional comments (20)
src/weac/logging_config.py (1)

9-9: LGTM! Modern type hint syntax adopted.

The migration from Optional[str] to str | None aligns with PEP 604 and Python 3.10+ union syntax, improving readability.

src/weac/components/model_input.py (1)

51-57: LGTM! Built-in generic types adopted.

The migration from List[...] to list[...] is consistent with Python 3.9+ built-in generic syntax and aligns with the broader typing modernization across the codebase.

src/weac/core/slab_touchdown.py (1)

7-7: LGTM! Union type syntax adopted.

The migration from Optional[float] to float | None is consistent with Python 3.10+ union syntax. No behavioral changes detected.

Also applies to: 63-63, 81-81

tests/utils/weac_reference_runner.py (1)

49-49: LGTM! Built-in generic types adopted.

The migration from Dict, Tuple, Optional to built-in dict, tuple, and union syntax (|) is consistent with Python 3.9+ and improves code clarity.

Also applies to: 76-76, 296-298

src/weac/utils/snowpilot_parser.py (1)

49-49: LGTM! Built-in generic types adopted.

The migration from List[...], Tuple[...] to list[...], tuple[...] is consistent with Python 3.9+ built-in generic syntax and aligns with the broader typing modernization.

Also applies to: 52-70, 215-215, 222-222, 275-276

src/weac/analysis/criteria_evaluator.py (2)

36-41: LGTM! Built-in generic types and union syntax adopted.

The migration from List[...], Optional[...] to list[...] and union syntax (|) is consistent with Python 3.10+ type hints and aligns with the broader typing modernization.

Also applies to: 91-91, 132-135, 146-150, 205-209


831-831: LGTM! Return type annotations modernized.

The updates to return types using tuple[...] and list[...] are consistent with Python 3.9+ built-in generic syntax.

Also applies to: 844-844, 934-934, 950-950, 1092-1092

src/weac/core/unknown_constants_solver.py (1)

11-11: LGTM! Union type syntax adopted.

The migration from Optional[...] to union syntax (|) for touchdown-related parameters is consistent with Python 3.10+ type hints.

Also applies to: 39-42, 266-268, 370-372

src/weac/core/eigensystem.py (1)

59-71: LGTM! NumPy type annotations modernized.

The migration from NDArray to explicit np.ndarray type annotations and from Optional[float] to float | None improves type clarity and aligns with Python 3.10+ union syntax.

Also applies to: 115-116, 172-180, 216-218, 278-278, 360-360

src/weac/core/scenario.py (2)

46-51: Scenario.segments type hints now consistent with ModelInput and Segment usage

Switching segments to list[Segment] at both the attribute and __init__ parameter matches ModelInput.segments (also list[Segment]) and keeps runtime behavior unchanged while improving type clarity. Looks good.

Also applies to: 69-80


6-7: Modernized typing for get_segment_idx is accurate and coherent

Importing Sequence from collections.abc and annotating x as float | Sequence[float] | np.ndarray with return type int | np.ndarray correctly reflects the existing np.asarray + scalar/array handling, and the docstring was updated accordingly. No functional change, just clearer types.

Also applies to: 104-118

src/weac/core/system_model.py (3)

120-137: Touchdown-related typing cleanup is consistent with existing behavior

Annotating slab_touchdown (attribute and cached_property) as SlabTouchdown | None and allowing config: Config | None = None in __init__ aligns the public API with actual usage (None when touchdown disabled, concrete instance otherwise) without changing logic. This also matches how unknown_constants and uncracked_unknown_constants branch on self.slab_touchdown is not None. Looks solid.

Also applies to: 145-155, 156-195


292-315: update_layers now exposes concrete list[Layer] type

The updated signature update_layers(self, new_layers: list[Layer]) matches how Slab is constructed (Slab(layers=new_layers)) and how ModelInput.layers is typed. No behavior change; just tighter typing. Good.


317-341: update_scenario typing aligns with optional override semantics

Using segments: list[Segment] | None and scenario_config: ScenarioConfig | None matches the implementation that falls back to self.scenario.* when None, and keeps cache invalidation logic intact. No issues here.

src/weac/analysis/plotter.py (6)

9-9: Literal import correctly supports constrained field argument

Keeping from typing import Literal is appropriate for the field: Literal[...] parameter in plot_deformed; no issues here.

Also applies to: 804-807


220-237: _get_systems_to_plot typing and validation are consistent

The signature system_model: SystemModel | None / system_models: list[SystemModel] | None with a list[SystemModel] return type matches actual usage. The explicit guard against both arguments being set, and the final ValueError when neither is provided, make the helper’s contract clear and fail-fast. Looks good.


239-248: _save_figure now safely handles optional fig parameter

Allowing fig: Figure | None and defaulting to plt.gcf() when None keeps existing behavior but makes the helper more generally usable and better typed. The isnotebook()-based close logic is unchanged. No concerns.


643-650: plot_section_forces typing modernized and consistent with helper

The updated signature using system_model: SystemModel | None, system_models: list[SystemModel] | None, plus labels: list[str] | None and colors: list[str] | None aligns with _get_systems_to_plot and the downstream plotting loop. Runtime behavior is unchanged; typing is clearer. Looks good.

Also applies to: 656-666, 667-675


722-729: plot_energy_release_rates typing mirrors section_forces and is sound

This function’s updated union types for single vs multiple SystemModel inputs and optional labels/colors follow the same pattern as plot_section_forces, and the use of _get_systems_to_plot ensures consistent behavior. No issues here.

Also applies to: 735-745, 746-754


1033-1053: Stress envelope plotting: filename and method typing match usage

Allowing filename: str | None = None (with if filename: before saving) provides a simple way to skip saving while keeping backward-compatible behavior. The local helper find_sigma_for_tau(tau_val, sigma_c, method: str | None = None) now reflects that method can be None, consistent with passing it through to criteria_evaluator.stress_envelope. Envelope construction and error handling via brentq remain unchanged. This is a clean typing improvement.

Also applies to: 1077-1099, 1101-1151, 1152-1189

@pillowbeast pillowbeast changed the title Bug fix/sserr Bug: Fix SSERR evaluation to always use the flat config in line with current implementation Nov 24, 2025
@pillowbeast
Copy link
Collaborator Author

@coderabbitai configuration

@coderabbitai
Copy link

coderabbitai bot commented Nov 24, 2025

CodeRabbit configuration
language: en-US
tone_instructions: ''
early_access: true
enable_free_tier: true
reviews:
  profile: chill
  request_changes_workflow: false
  high_level_summary: true
  high_level_summary_instructions: ''
  high_level_summary_placeholder: '@coderabbitai summary'
  high_level_summary_in_walkthrough: false
  auto_title_placeholder: '@coderabbitai'
  auto_title_instructions: ''
  review_status: true
  commit_status: true
  fail_commit_status: false
  collapse_walkthrough: true
  changed_files_summary: true
  sequence_diagrams: true
  estimate_code_review_effort: true
  assess_linked_issues: true
  related_issues: true
  related_prs: true
  suggested_labels: true
  labeling_instructions: []
  auto_apply_labels: false
  suggested_reviewers: true
  auto_assign_reviewers: false
  in_progress_fortune: true
  poem: false
  path_filters: []
  path_instructions: []
  abort_on_close: true
  disable_cache: false
  auto_review:
    enabled: true
    auto_incremental_review: true
    ignore_title_keywords: []
    labels: []
    drafts: false
    base_branches: []
    ignore_usernames: []
  finishing_touches:
    docstrings:
      enabled: true
    unit_tests:
      enabled: true
  pre_merge_checks:
    docstrings:
      mode: warning
      threshold: 80
    title:
      mode: warning
      requirements: ''
    description:
      mode: warning
    issue_assessment:
      mode: warning
    custom_checks: []
  tools:
    ast-grep:
      rule_dirs: []
      util_dirs: []
      essential_rules: true
      packages: []
    shellcheck:
      enabled: true
    ruff:
      enabled: true
    markdownlint:
      enabled: true
    github-checks:
      enabled: true
      timeout_ms: 90000
    languagetool:
      enabled: true
      enabled_rules: []
      disabled_rules: []
      enabled_categories: []
      disabled_categories: []
      enabled_only: false
      level: picky
    biome:
      enabled: true
    hadolint:
      enabled: true
    swiftlint:
      enabled: true
    phpstan:
      enabled: true
      level: default
    phpmd:
      enabled: true
    phpcs:
      enabled: true
    golangci-lint:
      enabled: true
    yamllint:
      enabled: true
    gitleaks:
      enabled: true
    checkov:
      enabled: true
    detekt:
      enabled: true
    eslint:
      enabled: true
    flake8:
      enabled: true
    rubocop:
      enabled: true
    buf:
      enabled: true
    regal:
      enabled: true
    actionlint:
      enabled: true
    pmd:
      enabled: true
    clang:
      enabled: true
    cppcheck:
      enabled: true
    semgrep:
      enabled: true
    circleci:
      enabled: true
    clippy:
      enabled: true
    sqlfluff:
      enabled: true
    prismaLint:
      enabled: true
    pylint:
      enabled: true
    oxc:
      enabled: true
    shopifyThemeCheck:
      enabled: true
    luacheck:
      enabled: true
    brakeman:
      enabled: true
    dotenvLint:
      enabled: true
    htmlhint:
      enabled: true
    checkmake:
      enabled: true
    osvScanner:
      enabled: true
chat:
  art: true
  auto_reply: true
  integrations:
    jira:
      usage: auto
    linear:
      usage: auto
knowledge_base:
  opt_out: false
  web_search:
    enabled: true
  code_guidelines:
    enabled: true
    filePatterns: []
  learnings:
    scope: auto
  issues:
    scope: auto
  jira:
    usage: auto
    project_keys: []
  linear:
    usage: auto
    team_keys: []
  pull_requests:
    scope: auto
  mcp:
    usage: auto
    disabled_servers: []
code_generation:
  docstrings:
    language: en-US
    path_instructions: []
  unit_tests:
    path_instructions: []
issue_enrichment:
  planning:
    enabled: false

@pillowbeast pillowbeast merged commit 7bff613 into main Dec 4, 2025
4 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Dec 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants