[Fixes / Docs]: Fix a sign error, add linter, type checker, and add examples to README#19
Conversation
…ype checking - Added Clippy linting configuration to `Cargo.toml` to allow specific lints. - Updated `pyproject.toml` to include new dependencies: `pyright` for type checking and `ruff` for linting. - Enhanced GitHub Actions workflow to include a new linting job that runs Rust and Python linting checks before tests. - Made minor formatting adjustments in various Python scripts for consistency and clarity.
…r scale normalization - Updated axis limits to use dynamic extent values for better visualization. - Added vmin and vmax parameters to the color mapping for improved clarity in mode field plots.
… example and add corresponding image
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR bumps MicroMode to version 0.1.0a4, fixes a y-normal field mapping sign bug affecting power overlap, refines Python type annotations and the PML/Materials API surface, adds stricter sequence validation via Changes0.1.0a4 Release with Field Mapping Fix and API Refinement
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
QuentinWach
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 @.github/workflows/tests.yml:
- Around line 18-26: The workflow uses tag-pinned actions and leaves checkout
credentials persisted; update the steps using actions/checkout and
astral-sh/setup-uv (and any other action usages) to use commit SHAs instead of
tags and add persist-credentials: false to the actions/checkout step;
specifically locate the checkout step named "uses: actions/checkout@v4" and set
persist-credentials: false, and replace "uses: astral-sh/setup-uv@v5" (and any
other tag-pinned actions such as those for rust tooling if applicable) with
their corresponding full commit SHA refs to harden supply-chain integrity.
In `@examples/ridge_waveguide_readme.py`:
- Around line 104-111: The functions centered_edges and offset_edges can produce
invalid grids or divide-by-zero for nonpositive or too-large step values; add
input guards: validate that step > 0, compute cells = round(width / step) (or
round((upper - lower) / step)) and ensure cells >= 1 (and for offset_edges also
verify upper > lower), and raise a clear ValueError if these checks fail before
calling np.linspace so linspace always receives a positive count (cells + 1) and
valid bounds.
In `@examples/tidy3d_modal_sources_monitors.py`:
- Around line 93-95: The centered_edges function should validate inputs: ensure
step is positive (step > 0) to avoid divide-by-zero and compute cells =
round(width / step) then require cells >= 1; if these checks fail, raise a
ValueError with a clear message referencing width and step. Update
centered_edges to perform these validations before calling np.linspace so bad
CLI inputs fail fast and produce an actionable error.
In `@python/micromode/result.py`:
- Line 335: The assignment to solver_info currently wraps the HDF5 attribute in
str(), which mangles bytes payloads (e.g. b'{"ok":true}') and breaks json.loads;
update the solver_info assignment (the line creating solver_info from
handle.attrs["solver_info"]) to pass the raw attribute value to json.loads
(i.e., remove the str() coercion) so bytes returned by h5py are handled
correctly.
In `@scripts/check_release_metadata.py`:
- Around line 10-12: The fallback import for tomli in
scripts/check_release_metadata.py lacks the type-check ignore; update the
fallback import statement "import tomli as tomllib" to include the same comment
as the primary import by adding "# type: ignore[import-not-found]" so pyright
won't error during type-checking.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9cec96f-49fa-4ff8-9690-cda8d1b6f153
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockdocs/assets/hybridization_sweep.pngis excluded by!**/*.pngdocs/assets/ridge_modes.pngis excluded by!**/*.pngdocs/assets/tidy3d_modal_modes.pngis excluded by!**/*.pnguv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.github/workflows/tests.yml.gitignoreCHANGELOG.mdCargo.tomlREADME.mdbenchmarks/compare_mode_solver_fixtures.pybenchmarks/mode_solver/fixtures.pyexamples/README.mdexamples/material_grid_demos.pyexamples/ridge_waveguide_readme.pyexamples/soi_hybridization_sweep.pyexamples/tidy3d_modal_sources_monitors.pypyproject.tomlpython/micromode/_rust.pypython/micromode/models.pypython/micromode/raster.pypython/micromode/result.pypython/micromode/sweep.pyscripts/check_dist_artifacts.pyscripts/check_release_metadata.pyscripts/generate_coverage_badge.pyscripts/smoke_dist.pyscripts/smoke_wheel.pysrc/derivatives.rssrc/eigensolve.rssrc/sparse_matrix.rstests/test_micromode_api.pytests/test_mode_solver_fixtures.py
💤 Files with no reviewable changes (1)
- scripts/smoke_dist.py
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Install Rust tooling | ||
| run: rustup component add rustfmt clippy | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v5 | ||
| with: | ||
| version: "latest" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unpinned action refs and checkout credential persistence in workflow files.
rg -n 'uses:\s*[^ ]+@v[0-9]+' .github/workflows/tests.yml
rg -n -A3 -B1 'uses:\s*actions/checkout@' .github/workflows/tests.ymlRepository: QuentinWach/micromode
Length of output: 1232
Pin action SHAs and disable checkout credential persistence.
The actions at these lines are tag-pinned (@v4, @v5) rather than commit-pinned, and actions/checkout lacks persist-credentials: false. This weakens CI supply-chain hardening.
Suggested hardening
- - uses: actions/checkout@v4
+ - uses: actions/checkout@<pinned-commit-sha>
+ with:
+ persist-credentials: false
- - name: Install uv
- uses: astral-sh/setup-uv@v5
+ - name: Install uv
+ uses: astral-sh/setup-uv@<pinned-commit-sha>
with:
version: "latest"Note: This pattern appears throughout the workflow file; consider applying the same hardening to all action usages.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 18-18: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 18-18: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 24-24: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for 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.
In @.github/workflows/tests.yml around lines 18 - 26, The workflow uses
tag-pinned actions and leaves checkout credentials persisted; update the steps
using actions/checkout and astral-sh/setup-uv (and any other action usages) to
use commit SHAs instead of tags and add persist-credentials: false to the
actions/checkout step; specifically locate the checkout step named "uses:
actions/checkout@v4" and set persist-credentials: false, and replace "uses:
astral-sh/setup-uv@v5" (and any other tag-pinned actions such as those for rust
tooling if applicable) with their corresponding full commit SHA refs to harden
supply-chain integrity.
| def centered_edges(*, width: float, step: float) -> np.ndarray: | ||
| cells = round(width / step) | ||
| return np.linspace(-0.5 * width, 0.5 * width, cells + 1) |
There was a problem hiding this comment.
Validate step and enforce at least one cell in centered_edges.
round(width / step) can produce invalid grids (or divide-by-zero) for bad CLI inputs; fail fast with a clear error.
Suggested fix
def centered_edges(*, width: float, step: float) -> np.ndarray:
+ if not np.isfinite(step) or step <= 0:
+ raise ValueError("step must be finite and > 0")
cells = round(width / step)
+ if cells < 1:
+ raise ValueError("step is too large for width; expected at least one cell")
return np.linspace(-0.5 * width, 0.5 * width, cells + 1)📝 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.
| def centered_edges(*, width: float, step: float) -> np.ndarray: | |
| cells = round(width / step) | |
| return np.linspace(-0.5 * width, 0.5 * width, cells + 1) | |
| def centered_edges(*, width: float, step: float) -> np.ndarray: | |
| if not np.isfinite(step) or step <= 0: | |
| raise ValueError("step must be finite and > 0") | |
| cells = round(width / step) | |
| if cells < 1: | |
| raise ValueError("step is too large for width; expected at least one cell") | |
| return np.linspace(-0.5 * width, 0.5 * width, cells + 1) |
🤖 Prompt for 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.
In `@examples/tidy3d_modal_sources_monitors.py` around lines 93 - 95, The
centered_edges function should validate inputs: ensure step is positive (step >
0) to avoid divide-by-zero and compute cells = round(width / step) then require
cells >= 1; if these checks fail, raise a ValueError with a clear message
referencing width and step. Update centered_edges to perform these validations
before calling np.linspace so bad CLI inputs fail fast and produce an actionable
error.
| import tomllib # type: ignore[import-not-found] | ||
| except ModuleNotFoundError: # pragma: no cover - Python 3.10 compatibility. | ||
| import tomli as tomllib |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether tomli is declared anywhere and inspect type-check config.
rg -n --iglob 'pyproject.toml' --iglob 'requirements*.txt' --iglob '*lock*' '\btomli\b|requires-python|pyright'Repository: QuentinWach/micromode
Length of output: 11043
🏁 Script executed:
cat -n pyproject.toml | sed -n '90,110p'Repository: QuentinWach/micromode
Length of output: 357
🏁 Script executed:
cat -n scripts/check_release_metadata.py | head -20Repository: QuentinWach/micromode
Length of output: 669
🏁 Script executed:
rg -A 20 '\[tool\.pyright\]' pyproject.tomlRepository: QuentinWach/micromode
Length of output: 225
Add # type: ignore[import-not-found] to the fallback tomli import on line 12.
The fallback import at line 12 needs the same type-checking ignore as line 10. Even though tomli is declared in pyproject.toml as a conditional dev dependency (python_version < '3.11'), pyright configured in basic mode at Python 3.10 doesn't recognize it as available during type-checking. Add the matching ignore comment to make CI pass.
Suggested patch
try:
import tomllib # type: ignore[import-not-found]
except ModuleNotFoundError: # pragma: no cover - Python 3.10 compatibility.
- import tomli as tomllib
+ import tomli as tomllib # type: ignore[import-not-found]🧰 Tools
🪛 GitHub Actions: Tests / 4_Lint and type check.txt
[error] 12-12: pyright: Import "tomli" could not be resolved (reportMissingImports).
🪛 GitHub Actions: Tests / Lint and type check
[error] 12-12: pyright: Import "tomli" could not be resolved (reportMissingImports)
🤖 Prompt for 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.
In `@scripts/check_release_metadata.py` around lines 10 - 12, The fallback import
for tomli in scripts/check_release_metadata.py lacks the type-check ignore;
update the fallback import statement "import tomli as tomllib" to include the
same comment as the primary import by adding "# type: ignore[import-not-found]"
so pyright won't error during type-checking.
- Updated GitHub Actions workflows to disable credential persistence during checkout for better security. - Added input validation in `centered_edges` and `offset_edges` functions to ensure positive step sizes and valid width/upper-lower relationships. - Refactored solver info access in tests for improved clarity and maintainability.
1 participant
Summary by CodeRabbit
Release Notes v0.1.0-alpha.4
New Features
Bug Fixes
Documentation
Chores