Skill review: fix doc/code contradictions, restructure, add validation/dry-run/preflight/sweep - #1
Merged
Merged
Conversation
…th reference CSV - grating-skill/ -> scripts/; BUGS_AND_FIXES.md -> references/; Pitfalls guide -> references/legacy/ - Remove committed .mph (11.8 MB) + .mat (6.4 MB) reference binaries; commit the extracted per-order CSV (examples/) as the known-good reference instead (R=0.118655, T=0.881345, R+T=1.000000) - .gitignore: ignore simulation outputs (*.mph, *.mat, *_orders.csv) but keep examples/*.csv Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…p, tests Hard fixes: - SKILL.md claimed complex n was accepted; the script float()-crashed on it. Docs now state lossless real-index only, and validate_config() rejects complex n with a clear message (metals/lossy gratings are out of scope) - Pitfalls guide §2 over-claimed that Box selectors accept only pure numeric strings; corrected: parameter-name expressions fail silently, unit-suffixed literals (the template's own style, Bug-4) work - Add missing MIT LICENSE (readme_zh linked it; README said 'if present') - Mark internal provenance docs as not included in the repo (SKILL.md References + BUGS_AND_FIXES.md pointed at files that do not exist here) - README console example showed fabricated multi-order output for the sub-wavelength example config; replaced with the real reference numbers Enhancements: - validate_config(): fail fast before JVM/license cost on bad labels (non-identifier, duplicate, reserved air/groove), duty/theta/N_teeth ranges, complex n; warns when d_air < 2*lambda - --dry-run: orders/ports/filenames report without COMSOL - --preflight: mph + COMSOL + license environment check - --sweep key=start:stop:step (or list): single COMSOL session reused across cases, per-case outputs + aggregated _sweep CSV/MAT - np.trapezoid/np.trapz compat (trapz removed in NumPy 2.0; the Poynting cross-check silently skipped on mismatched versions) - Port outside-index: warn if neither 'n' nor 'RefIndex' could be set instead of silently swallowing both failures - hmax_factor config key for mesh-convergence checks (default 6, unchanged) - ASCII-only runtime messages (em-dashes rendered as mojibake on GBK consoles); fixed stale comment and shadowed variable in Poynting block - tests/: 30 COMSOL-free unit tests for validation, order math, sweep parsing - SKILL.md: pushier trigger description + compatibility field, Step 0 preflight, material-name -> n quick table, expanded When-NOT-to-use (non-rectangular profiles, conical incidence, substrate-side illumination, metals), sweep workflow, troubleshooting map - READMEs (en/zh) synced to new layout; zh quick-start JSON no longer contains // comments (was invalid JSON if copied verbatim) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR brings the repository into closer alignment with the “skill” spec by restructuring files, fixing documentation/code contradictions, and adding COMSOL-free validation/dry-run/preflight/sweep functionality around the existing (unchanged) COMSOL physics recipe.
Changes:
- Restructures the repo layout (moves runnable template into
scripts/, references intoreferences/, addsexamples/reference outputs) and updates docs accordingly. - Adds config validation,
--dry-run,--preflight, and single-session parameter sweeps, plus NumPy 2.0 compatibility handling. - Adds COMSOL-free unit tests for validation, propagating-order math, and sweep parsing.
Reviewed changes
Copilot reviewed 11 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/comsol_grating_template.py |
New main runnable template with validation, dry-run/preflight, sweep support, and post-processing/export logic. |
tests/test_validation.py |
Adds COMSOL-free unittest coverage for validator, order math, and sweep parsing. |
SKILL.md |
Updates skill metadata + usage workflow; documents new flags, sweeps, and troubleshooting map. |
README.md |
Refreshes quick-start and corrects example console output; documents new layout and features. |
readme_zh.md |
Syncs Chinese quick-start and docs with new layout/flags and valid JSON examples. |
references/BUGS_AND_FIXES.md |
Clarifies provenance and “internal docs not included” note. |
references/legacy/COMSOL_MATLAB_Pitfalls_Guide.md |
Marks legacy status and corrects over-broad Box-selector claims. |
scripts/requirements.txt |
Adds Python dependency list for running the template. |
scripts/example_config.json |
Adds a minimal runnable example config. |
scripts/example_run.bat |
Adds a Windows convenience launcher for the example config. |
examples/grating_N1_theta0.00_orders.csv |
Commits a small “known-good” reference output for installation verification. |
examples/README.md |
Documents how to use the committed reference output for verification. |
LICENSE |
Adds MIT license file to match README claims. |
.gitignore |
Ignores large simulation outputs while preserving committed reference CSVs under examples/. |
grating-skill/comsol_grating_template.py |
Removes the old script location in favor of the new scripts/ layout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+358
to
+362
| top_orders = propagating_orders(sin_i, n_air, W, lam) | ||
| bot_orders = propagating_orders(sin_i, n_substrate, W, lam) | ||
| print(f"[INFO] propagating orders: top air={len(top_orders)} " | ||
| f"({top_orders[0]}..{top_orders[-1]}), bot {layers[0][2]} n={n_substrate}: " | ||
| f"{len(bot_orders)} ({bot_orders[0]}..{bot_orders[-1]})") |
Comment on lines
+900
to
+903
| fbase = f"{case['out_prefix']}_N{N}_theta{theta:.2f}" | ||
| print(f"[DRY-RUN]{tag} W={W:g} nm, cell height={height:g} nm | " | ||
| f"orders: top {len(top)} ({top[0]}..{top[-1]}), " | ||
| f"bot {len(bot)} ({bot[0]}..{bot[-1]}) -> {len(top) + len(bot)} ports") |
Comment on lines
+925
to
+928
| except ImportError as exc: | ||
| print(f"[PREFLIGHT] FAIL: {exc}") | ||
| print(" fix: pip install -r requirements.txt") | ||
| return 1 |
Comment on lines
+61
to
+66
| try: | ||
| import numpy as np | ||
| import scipy.io as sio | ||
| except ImportError: | ||
| print("[ERROR] Required packages missing: pip install numpy scipy") | ||
| sys.exit(1) |
Comment on lines
158
to
160
| 2. `[CHECK] Poynting net flux` top should be **negative** (energy going down, | ||
| i.e. away from source) and bot should match it within ~1%. This is an | ||
| independent cross-check of R−T. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
背景
对照 skill 编写规范做了一次完整评审,修复发现的问题并补齐功能缺口。已验证的 COMSOL 物理代码一行未改(见下方验证)。
硬伤修复(文档与代码/仓库不一致)
float()强转会直接崩溃。现在文档明确"仅支持无损实数 n",且validate_config()对复数 n 给出清晰报错"574[nm]",66 例验证通过)矛盾。已更正为:仅含参数名的表达式会静默归零,字面量+单位可用结构重构
grating-skill/→scripts/,文档 →references/(遗留指南进legacy/),符合 skill 标准布局.mph/.mat二进制,改为提交从中提取的 1 KB 参考 CSV(examples/);.gitignore相应调整(原来反而把最有参考价值的 CSV 忽略了)功能增强
validate_config():非法/重复/保留(air/groove)层标签、duty/θ/N_teeth 越界、复数 n 等在启动 COMSOL 之前一次性报全;d_air < 2λ 告警--dry-run:免 license 验证配置并打印传播级次/端口数/输出文件名--preflight:环境预检(mph、COMSOL、license)--sweep key=start:stop:step:波长/角度/几何扫描,整个扫描复用一个 COMSOL 会话,输出逐例文件 + 聚合_sweepCSV/MATnp.trapz已被移除,旧写法会让 Poynting 交叉验证被静默跳过)hmax_factor网格密度配置项(默认 6,行为不变)tests/:30 个免 COMSOL 单元测试//注释(照抄会解析失败),已改为合法 JSON + 旁注验证(本机 COMSOL 6.4 实测)
--preflight通过🤖 Generated with Claude Code