Bug fix/sserr by pillowbeast · Pull Request #32 · 2phi/weac

pillowbeast · 2025-12-04T10:09:48Z

Fix Sphinx Docs

Summary by CodeRabbit

Documentation
- Expanded README with a new Development Setup section covering environment configuration and tooling instructions.
Chores
- Modernized Python type annotations throughout the codebase for improved IDE support and type-checking compatibility.
- Added explicit Python version specification (3.12).
- Updated CI/CD workflows and dependency management tooling.
- Reorganized optional dependencies for build tools.

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

coderabbitai · 2025-12-04T10:09:59Z

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

This pull request modernizes the project's Python type annotations to use PEP 604 syntax (replacing typing.Optional with | None and typing.Union with |), upgrades CI workflows to use UV package manager instead of pip, adds Python version pinning, reorganizes build dependencies, updates SSERR evaluation logic for touchdown configurations, and adds validation checks in plotting functions.

Changes

Cohort / File(s)	Summary
Workflow Toolchain Upgrade `.github/workflows/docs.yml`, `format.yml`, `pylint.yml`, `release.yml`, `tests.yml`	Upgraded actions/checkout from v4 to v6, replaced Python setup with astral-sh/setup-uv@v7, replaced pip with uv commands (uv sync, uv run), and removed explicit Python version pinning in favor of version: "latest".
Python Version and Build Configuration `.gitignore`, `.python-version`, `pyproject.toml`	Removed .python-version from .gitignore, added new .python-version file with "3.12", created new `build` optional dependency group in pyproject.toml containing `build` and `twine` packages.
Type Annotation Modernization `src/weac/analysis/criteria_evaluator.py`, `plotter.py`, `src/weac/components/model_input.py`, `src/weac/core/eigensystem.py`, `scenario.py`, `slab.py`, `slab_touchdown.py`, `system_model.py`, `unknown_constants_solver.py`, `src/weac/logging_config.py`, `src/weac/utils/snowpilot_parser.py`, `tests/utils/weac_reference_runner.py`	Replaced typing imports (List, Optional, Union, Tuple, Dict) with built-in generics (list, dict, tuple) and PEP 604 union syntax (X \| Y, X \| None) across method signatures and class attributes.
SSERR Evaluation and Validation Logic `src/weac/analysis/criteria_evaluator.py`, `plotter.py`	Updated SSERR evaluation to force touchdown configuration with phi=0.0 and use 2*l_BC segment length; added validation checks in plot_slab_profile to ensure weak_layers match slabs count and label length consistency.
Test Baseline and Configuration Updates `tests/analysis/test_criteria_evaluator.py`, `tests/test_regression_simulation.py`	Updated tests to use Config(touchdown=True) instead of default config; adjusted regression baselines for touchdown_distance and SSERR values to reflect new configuration.
Documentation `README.md`	Reorganized table of contents and added Development Setup section with UV setup and workflow instructions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Areas requiring extra attention:

SSERR evaluation changes (src/weac/analysis/criteria_evaluator.py): Verify that the forced touchdown configuration (phi=0.0, 2*l_BC segment length, l_BC derivation from slab_touchdown) is correct and does not introduce unintended side effects.
Test baseline updates (tests/test_regression_simulation.py): Confirm that the new regression values (touchdown_distance: 1265.551924690803, SSERR: 2.123992) are accurate and represent correct behavior rather than masking a regression.
Validation logic in plotter (src/weac/analysis/plotter.py): Ensure the new ValueError checks for weak_layers/slabs count and labels length are appropriate and do not break existing calling patterns.
Type annotation consistency: Spot-check a few key signatures (especially in system_model.py and criteria_evaluator.py) to ensure union syntax is applied uniformly and imports are correctly removed.

Possibly related PRs

Bug: Fix SSERR evaluation to always use the flat config in line with current implementation #31: Modifies SSERR evaluation path in criteria_evaluator.py with similar touchdown configuration and segment handling adjustments.
Fix documentation workflow for proper GitHub Pages deployment #21: Updates .github/workflows/docs.yml with build and deployment workflow changes.

Suggested labels

bug

Suggested reviewers

zacczakk

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch bug-fix/SSERR

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

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 7bff613 and 99421fc.

⛔ Files ignored due to path filters (1)

uv.lock is excluded by !**/*.lock

📒 Files selected for processing (23)

.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)
pyproject.toml (1 hunks)
src/weac/analysis/criteria_evaluator.py (11 hunks)
src/weac/analysis/plotter.py (11 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 (6 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/analysis/test_criteria_evaluator.py (1 hunks)
tests/test_regression_simulation.py (2 hunks)
tests/utils/weac_reference_runner.py (4 hunks)

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

Pull request overview

This PR modernizes type hints to use Python 3.10+ native syntax (replacing typing.List, typing.Dict, etc. with list, dict), fixes SSERR evaluation logic, migrates CI/CD workflows to use uv for faster dependency management, and adds comprehensive development setup documentation.

Key Changes:

Modernized type hints across the codebase to use native Python 3.10+ syntax (list[T] instead of List[T])
Updated SSERR evaluation to properly configure touchdown mode and use correct segment lengths
Migrated all GitHub Actions workflows from pip to uv for dependency management
Added detailed development setup guide in README including uv installation and usage

Reviewed changes

Copilot reviewed 22 out of 24 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
tests/utils/weac_reference_runner.py	Modernized type hints from `typing` module to native Python 3.10+ syntax
tests/test_regression_simulation.py	Updated test to use `Config(touchdown=True)` and adjusted baseline values for SSERR calculations
tests/analysis/test_criteria_evaluator.py	Updated test to use `Config(touchdown=True)` for SSERR evaluation
src/weac/utils/snowpilot_parser.py	Modernized type hints from `typing.List/Tuple` to native `list`/`tuple` syntax
src/weac/logging_config.py	Modernized type hint from `Optional[str]` to `str \| None`
src/weac/core/unknown_constants_solver.py	Modernized type hints and reformatted multi-line type annotations
src/weac/core/system_model.py	Modernized type hints and updated isinstance check to use `Sequence`
src/weac/core/slab_touchdown.py	Modernized type hints from `Optional` to union syntax
src/weac/core/slab.py	Modernized `List[Layer]` to `list[Layer]` type hints
src/weac/core/scenario.py	Modernized type hints and imported `Sequence` from `collections.abc`
src/weac/core/eigensystem.py	Modernized type hints, though introduced invalid `np.ndarray[dtype]` syntax
src/weac/components/model_input.py	Modernized type hints and updated field descriptions
src/weac/analysis/plotter.py	Modernized type hints and added validation for matching weak layer/slab counts
src/weac/analysis/criteria_evaluator.py	Modernized type hints, updated SSERR logic with proper touchdown configuration and segment lengths
pyproject.toml	Moved build tools from dev dependencies to separate `build` extra
README.md	Added comprehensive development setup section documenting uv usage
.python-version	Added Python version specification file (3.12)
.gitignore	Removed `.python-version` from gitignore to track it in repository
.github/workflows/tests.yml	Migrated from pip to uv, updated action versions (though to non-existent v6)
.github/workflows/release.yml	Migrated to uv for building and publishing
.github/workflows/pylint.yml	Migrated to uv for dependency installation
.github/workflows/format.yml	Migrated to uv for running ruff
.github/workflows/docs.yml	Migrated to uv for building Sphinx documentation

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

Copilot · 2025-12-04T10:16:08Z

-    layers: List[Layer] = Field(
-        default_factory=lambda: [Layer(rho=250, h=100)], description="List of layers"
+    layers: list[Layer] = Field(
+        default_factory=lambda: [Layer(rho=250, h=100)], description="list of layers"


[nitpick] The description changed from "List of layers" to "list of layers" (lowercase). While this matches the type hint change, it's inconsistent with other field descriptions like "Weak layer" (line 49) and "Scenario configuration" (line 55) which use capitalization. Consider keeping consistent capitalization across all field descriptions.

Suggested change

default_factory=lambda: [Layer(rho=250, h=100)], description="list of layers"

default_factory=lambda: [Layer(rho=250, h=100)], description="List of layers"

Copilot · 2025-12-04T10:16:09Z

+    ewC: np.ndarray[np.complex128]  # shape (k): Complex Eigenvalues
+    ewR: np.ndarray[np.float64]  # shape (k): Real Eigenvalues
+    evC: np.ndarray[np.complex128]  # shape (6, k): Complex Eigenvectors
+    evR: np.ndarray[np.float64]  # shape (6, k): Real Eigenvectors


The type hint syntax np.ndarray[np.complex128] and np.ndarray[np.float64] is invalid. NumPy arrays should be typed as np.ndarray (without dtype specification) or using the correct numpy typing syntax like npt.NDArray[np.complex128] (requires from numpy.typing import NDArray). The current syntax will cause type checking errors.

Copilot · 2025-12-04T10:16:09Z


    def calc_eigenvalues_and_eigenvectors(
-        self, system_matrix: NDArray[np.float64]
+        self, system_matrix: np.ndarray[np.float64]


The type hint np.ndarray[np.float64] is invalid. NumPy arrays should be typed as np.ndarray (without dtype specification) or using the correct numpy typing syntax like npt.NDArray[np.float64] (requires from numpy.typing import NDArray). The current syntax will cause type checking errors.

Copilot · 2025-12-04T10:16:09Z

            Solution vector (6xN) at position x.
        """
-        if isinstance(x, (list, tuple, np.ndarray)):
+        if isinstance(x, (np.ndarray, Sequence)):


The isinstance check order is problematic. np.ndarray should be checked before Sequence because np.ndarray is a Sequence in Python's abstract base class hierarchy. This could lead to incorrect behavior when the order matters. The original code had (list, tuple, np.ndarray) which was more explicit. Consider changing to (list, tuple, np.ndarray) or check np.ndarray first.

Suggested change

if isinstance(x, (np.ndarray, Sequence)):

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

Copilot · 2025-12-04T10:16:10Z

+            cut_length=2 * l_BC,
        )
-        system_copy.config.touchdown = True
+        # system_copy.config.touchdown = True


Commented-out code should be removed. Line 687 contains # system_copy.config.touchdown = True which is redundant since line 674 already sets this.

Suggested change

# system_copy.config.touchdown = True

Copilot · 2025-12-04T10:16:10Z

+    sR: np.ndarray[
        np.float64
    ]  # shape (k): Real positive eigenvalue shifts (for numerical robustness)
-    sC: NDArray[
+    sC: np.ndarray[
        np.float64
    ]  # shape (k): Complex positive eigenvalue shifts (for numerical robustness)


The type hint syntax np.ndarray[np.float64] is invalid. NumPy arrays should be typed as np.ndarray (without dtype specification) or using the correct numpy typing syntax like npt.NDArray[np.float64] (requires from numpy.typing import NDArray). The current syntax will cause type checking errors.

Copilot · 2025-12-04T10:16:10Z

-        self, kn: Optional[float], kt: Optional[float]
-    ) -> NDArray[np.float64]:
+        self, kn: float | None, kt: float | None
+    ) -> np.ndarray[np.float64]:


The return type hint np.ndarray[np.float64] is invalid. NumPy arrays should be typed as np.ndarray (without dtype specification) or using the correct numpy typing syntax like npt.NDArray[np.float64] (requires from numpy.typing import NDArray). The current syntax will cause type checking errors.

Copilot · 2025-12-04T10:16:10Z

+        np.ndarray[np.complex128],
+        np.ndarray[np.float64],
+        np.ndarray[np.complex128],
+        np.ndarray[np.float64],
+        np.ndarray[np.float64],
+        np.ndarray[np.float64],


The return type hints use np.ndarray[np.complex128] and np.ndarray[np.float64] which are invalid. NumPy arrays should be typed as np.ndarray (without dtype specification) or using the correct numpy typing syntax like npt.NDArray[np.complex128] (requires from numpy.typing import NDArray). The current syntax will cause type checking errors.

Copilot · 2025-12-04T10:16:11Z

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


This line accesses system.slab_touchdown.l_BC before ensuring that slab_touchdown is computed. If the original system was created with config.touchdown=False, then system.slab_touchdown will be None, causing an AttributeError. The touchdown is only enabled on system_copy in line 674, but line 676 accesses the original system. This should access system_copy.slab_touchdown.l_BC instead, or ensure touchdown is enabled on the original system first.

Suggested change

l_BC = system.slab_touchdown.l_BC

l_BC = system_copy.slab_touchdown.l_BC

pillowbeast added 13 commits November 24, 2025 11:39

Bug: SSERR works only in flat setup

6f6a182

formatting

9284d59

fix tests

1930951

change to uv for local development

5df6f4e

CI to uv

bb34b83

Docs fix

b136711

use uv sync consistently

0c684a1

evaluate_SSERR usable without even with touchdown = True not enabled.

80224eb

Replace List with list according to modern Python standard

a1c9b04

Update Typing to modern standards

a1d402c

use deterministic uv version, uv uses python from .python-version

780b5c0

Additional Comment fixes

1bfcb68

Sphinx Doc Fix

99421fc

Copilot AI review requested due to automatic review settings December 4, 2025 10:09

pillowbeast temporarily deployed to docs-deploy December 4, 2025 10:09 — with GitHub Actions Inactive

Copilot started reviewing on behalf of pillowbeast December 4, 2025 10:10 View session

pillowbeast merged commit f398363 into main Dec 4, 2025
9 of 10 checks passed

Copilot finished reviewing on behalf of pillowbeast December 4, 2025 10:13

Copilot AI reviewed Dec 4, 2025

View reviewed changes

pillowbeast deleted the bug-fix/SSERR branch December 4, 2025 10:34

coderabbitai Bot mentioned this pull request Dec 8, 2025

Tensile Strength Evaluation as part of Steady State Analysis #34

Merged

coderabbitai Bot mentioned this pull request Jan 12, 2026

Add slab tensile criterion to stress analysis + Fix #37 & more #38

Merged

coderabbitai Bot mentioned this pull request Feb 17, 2026

Feature/variational model for out-of-plane bending #42

Merged

	default_factory=lambda: [Layer(rho=250, h=100)], description="list of layers"
	default_factory=lambda: [Layer(rho=250, h=100)], description="List of layers"

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

	l_BC = system.slab_touchdown.l_BC
	l_BC = system_copy.slab_touchdown.l_BC

Conversation

pillowbeast commented Dec 4, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pillowbeast commented Dec 4, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Dec 4, 2025 •

edited

Loading