|
| 1 | +--- |
| 2 | +name: reviewing-openmc-code |
| 3 | +description: Reviews code changes in the OpenMC codebase against OpenMC's contribution criteria (correctness, testing, physics soundness, style, design, performance, docs, dependencies). Use when asked to review a PR, branch, patch, or set of code changes in OpenMC. |
| 4 | +--- |
| 5 | + |
| 6 | +Apply repository-wide guidance from `AGENTS.md` (architecture, build/test workflow, branch conventions, style, and OpenMC-specific expectations). |
| 7 | + |
| 8 | +## Determine Review Context |
| 9 | + |
| 10 | +1. **Identify what to review.** Determine the review context based on what the user provides or what can be inferred: |
| 11 | + - **Default (no explicit context):** Compare the current branch against `develop` using `git diff develop...HEAD` to see only commits unique to the current branch. |
| 12 | + - **User specifies a base branch or commit range:** Use that instead. |
| 13 | + - **PR context provided by tooling:** Use the base/head refs supplied by the tool. |
| 14 | +2. **Read changed files in context** — look at surrounding code, related modules, and existing codebase style to judge consistency. |
| 15 | +3. **Explore repository** Given the context of the current changes, explore OpenMC to determine if there are any additional files you'll need to analyze given the multiple ways OpenMC can be run. |
| 16 | + |
| 17 | +## Review Criteria |
| 18 | + |
| 19 | +Assess each of the following areas, noting any issues found. If an area looks good, briefly confirm it passes. |
| 20 | + |
| 21 | +### Purpose and Scope |
| 22 | +- Do the changes have a clear, well-defined purpose? |
| 23 | +- Are the changes of **general enough interest** to warrant inclusion in the main OpenMC codebase, or would they be better suited as a downstream extension? |
| 24 | + |
| 25 | +### Correctness and Testing |
| 26 | +- Do the changes compile and can you confirm all logic to be functionally correct? |
| 27 | +- Are appropriate **unit tests** added in `tests/unit_tests/` for new Python API features? |
| 28 | +- Are appropriate **regression tests** added in `tests/regression_tests/` for new simulation capabilities? |
| 29 | +- Are edge cases and error conditions handled and tested? |
| 30 | +- Are all changes sound when considering that OpenMC runs in parallel with MPI and OpenMP? |
| 31 | + |
| 32 | +### Physics Soundness (when applicable) |
| 33 | +- When the changes implement new physics, are the **equations, methods, and approaches physically sound**? |
| 34 | +- Are the algorithms consistent with established references? Are those references cited in comments or documentation? |
| 35 | +- Are there numerical stability or accuracy concerns with the implementation? |
| 36 | + |
| 37 | +### Code Quality and Style |
| 38 | +- Does the C++ code conform to the OpenMC style guide: `CamelCase` classes, `snake_case` functions/variables, trailing underscores for class members, C++17 idioms, `openmc::vector` instead of `std::vector`? |
| 39 | +- Does the Python code conform to PEP 8, use numpydoc docstrings, `pathlib.Path` for filesystem operations, and `openmc.checkvalue` for input validation? |
| 40 | +- Are the changes (API design, naming, abstractions, file organization) **consistent with the rest of the codebase**? |
| 41 | + |
| 42 | +### Design |
| 43 | +- Is the design as simple as it could be while still meeting the requirements? |
| 44 | +- Are there **alternative designs** that would achieve the same purpose with greater simplicity or better integration with existing infrastructure? |
| 45 | +- Does the API feel natural and follow the conventions established elsewhere in OpenMC? |
| 46 | + |
| 47 | +### Memory and Performance |
| 48 | +- Are there obvious memory leaks or unsafe memory management patterns in C++ code? |
| 49 | +- Do the changes introduce unnecessary performance regressions or greatly increased memory usage? |
| 50 | +- Do the changes introduce dynamic memory allocation (e.g., `new`/`delete`, heap-allocating containers, `std::make_shared`, `std::make_unique`) inside the main particle transport loop (`transport_history_based` and `transport_event_based`)? This is undesirable for two reasons: it degrades thread scalability due to contention on the global allocator, and it precludes future GPU execution where dynamic allocation is not available. |
| 51 | + |
| 52 | +### Documentation |
| 53 | +- Are new features, input parameters, and Python API additions **documented** (docstrings, `docs/source/`)? |
| 54 | +- Are new XML input attributes described in the input reference? |
| 55 | +- Are any deprecations or breaking changes clearly noted? |
| 56 | + |
| 57 | +### Dependencies |
| 58 | +- Do the changes introduce any new external software dependencies? |
| 59 | +- If so, are they justified, optional where possible, and consistent with OpenMC's existing dependency policy? |
| 60 | + |
| 61 | +## Output Format |
| 62 | + |
| 63 | +Produce your review as a structured report with the following sections: |
| 64 | + |
| 65 | +**Context**: State what is being compared (e.g., "current branch vs. `develop`", or the specific commit range/PR). |
| 66 | + |
| 67 | +**Summary**: A short paragraph describing what the changes do and your overall assessment. |
| 68 | + |
| 69 | +**Detailed Findings**: For each criterion above, provide a brief assessment. Use `✓` for items that pass and flag issues with severity: |
| 70 | +- `[Minor]` — Style nits, small improvements, non-blocking suggestions |
| 71 | +- `[Moderate]` — Issues worth addressing but not strictly blocking |
| 72 | +- `[Major]` — Problems that should be resolved before merging |
| 73 | + |
| 74 | +Group findings into: |
| 75 | +1. **Blocking issues** — Would justify requesting changes before merge |
| 76 | +2. **Non-blocking suggestions** — Improvements that could be addressed now or later |
| 77 | +3. **Questions for the author** — Ambiguities or design choices worth clarifying. Do not include questions that you are capable of answering yourself |
0 commit comments