From 4e37b244d4bb5eb85bd9130e8156769b789b7f4c Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Wed, 11 Feb 2026 15:36:26 -0500 Subject: [PATCH 1/7] Add developer guide, streamline PR template, and configure AI reviewers - Add docs/documentation/contributing.md as canonical developer guide with coding standards (hard rules + soft guidelines), fork-based workflow, testing, and PR process - Streamline PR template: fewer checkboxes, collapsible GPU section - Update copilot-instructions.md: soften size/arg limits, require GPU macros over raw pragmas - Add .coderabbit.yaml pointing to copilot-instructions.md via code_guidelines - Add .codeant/configuration.json for file include/exclude - Update .pr_agent.toml to reference coding standards - Slim .github/CONTRIBUTING.md to pointer to docs site - Link contributing page from docs readme Co-Authored-By: Claude Opus 4.6 --- .codeant/configuration.json | 14 ++++ .coderabbit.yaml | 25 ++++++ .github/CONTRIBUTING.md | 118 +++------------------------- .github/copilot-instructions.md | 24 +++--- .github/pull_request_template.md | 67 ++++++---------- .pr_agent.toml | 4 +- docs/documentation/contributing.md | 119 +++++++++++++++++++++++++++++ docs/documentation/readme.md | 4 + 8 files changed, 212 insertions(+), 163 deletions(-) create mode 100644 .codeant/configuration.json create mode 100644 .coderabbit.yaml create mode 100644 docs/documentation/contributing.md diff --git a/.codeant/configuration.json b/.codeant/configuration.json new file mode 100644 index 0000000000..35fd295c6f --- /dev/null +++ b/.codeant/configuration.json @@ -0,0 +1,14 @@ +{ + "review_configuration": { + "include": [ + "src/**/*.fpp", + "src/**/*.f90", + "toolchain/**/*.py" + ], + "exclude": [ + "tests/**", + "examples/**", + "docs/**" + ] + } +} diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 0000000000..b306a9d28f --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,25 @@ +# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json +language: en-us + +reviews: + profile: chill + path_instructions: + - path: "src/**/*.fpp" + instructions: | + Fortran source (Fypp-preprocessed). Follow the coding standards in + .github/copilot-instructions.md. Pay attention to naming conventions, + GPU compatibility (OpenACC), and error handling (s_mpi_abort, not stop). + - path: "src/**/*.f90" + instructions: | + Fortran source. Follow the coding standards in + .github/copilot-instructions.md. + - path: "toolchain/**/*.py" + instructions: | + Python toolchain code. Follow PEP 8. + +knowledge_base: + code_guidelines: + enabled: true + filePatterns: + - ".github/copilot-instructions.md" + - "docs/documentation/contributing.md" diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 71a68c5b16..c0c45a1470 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -1,113 +1,15 @@ -# Contributing to the MFC Codebase (Multi‑Component Flow Code) +# Contributing to MFC -**Multi‑Component Flow Code (MFC)** is an open‑source, high‑performance code for simulating compressible multi‑component, multi‑phase flows. -We welcome contributions of all kinds—bug fixes, new features, documentation, tests, and issue triage—from both newcomers and experienced developers. -This guide explains how to set up your environment, follow MFC's coding standards, and navigate the pull-request (PR) process so your work can be merged smoothly. +We welcome contributions of all kinds -- bug fixes, new features, documentation, tests, and issue triage. ---- +**Full developer guide:** [mflowcode.github.io/documentation/md_contributing.html](https://mflowcode.github.io/documentation/md_contributing.html) -## 1. Setting Up Your Development Environment +## Quick Reference -1. **Fork and clone** - ```bash - git clone https://github.com//MFC.git - cd MFC - git remote add upstream https://github.com/MFlowCode/MFC.git - ``` -2. **Build MFC** – follow the [documentation](https://mflowcode.github.io/documentation/md_getting-started.html). For example: - ```bash - ./mfc.sh build -j 8 # parallel build with 8 threads - ``` -3. **Run the test suite** to verify your environment: - ```bash - ./mfc.sh test -j 8 - ``` - ---- - -## 2. Development Workflow - -| Step | Action | Notes | -|------|--------|-------| -| 1 | **Sync your fork**: `git checkout master && git pull upstream master` | Stay up‑to‑date to avoid merge conflicts. | -| 2 | **Create a branch**: `git checkout -b feature/` | Keep each branch focused on one logical change. | -| 3 | **Code, test, document** | Follow the guidelines in §3. | -| 4 | **Format & lint**: `./mfc.sh format` | CI will re‑check; make it pass locally first. | -| 5 | **Run tests**: `./mfc.sh test` | All existing and new tests must pass. | -| 6 | **Commit** (see *Commit Messages* below) | Write clear, atomic commits. | -| 7 | **Push** & open a **PR** | Be mindful: *every push triggers CI*. Bundle fixes together to avoid dozens of CI runs. | - -### Commit Messages - -- Start with a concise (≤50 chars) summary in imperative mood: `Fix out‑of‑bounds in EOS module`. -- Add a blank line, then a detailed explanation. -- Reference related issues or PRs, e.g., `Fixes #123`. - -### Managing CI Runs - -Each push to a branch with an open PR runs the full CI matrix (which can take hours). -Plan your pushes—run tests locally and group changes—so the CI queue is not flooded. - ---- - -## 3. Coding Guidelines and Best Practices - -### 3.1 Style, Formatting & Linting -MFC enforces a project‑wide Fortran style: -- **Formatter**: `./mfc.sh format` auto‑formats your changes. -- **Linter**: CI runs several linter checks that spot common Fortran-gotchas (implicit typing, shadowed variables, unused locals, etc.). Fix issues before pushing or the linter will often catch them. - -### 3.2 Fypp Metaprogramming - -MFC uses [**Fypp**](https://github.com/aradi/fypp), a lightweight Python-based preprocessor, to generate repetitive or accelerator-specific Fortran. -Key points: -- Fypp macros live in `include/` directories nested within `src/`. -- Run `./mfc.sh format` to format the example case files and the source code. -- When editing `.fpp`, maintain readability, prefer simple macros over deeply nested constructs. - -### 3.3 Documentation - -- Add or update Doxygen comments in source files. -- Update Markdown docs under `docs/` if user‑facing behavior changes. -- Provide a minimal example in `examples/` for each new feature when practical. - -### 3.4 Testing - -- Add regression tests that fail before your change and pass after. -- Use `./mfc.sh test --generate` to create golden files for new cases. -- Keep tests fast; favor small grids and short runtimes. - -### 3.5 GPU & Performance - -- Ensure code compiles for CPU *and* GPU targets (NVHPC for NVIDIA, Cray for AMD). -- Profile critical kernels; avoid introducing bottlenecks. - ---- - -## 4. Preparing Your Pull Request - -1. **One PR = One logical change**. If you plan a follow‑up change, open an issue describing it and assign yourself for visibility. -2. **Fill out the PR template**. Remove checkboxes that do **not** apply. -3. **Describe testing** – list commands, compilers, and any profiling. -4. **Link issues** – `Fixes #` or `Part of #`. -5. **Ensure CI passes** before requesting review. - -> **Tip** If your change is large, consider splitting it into smaller PRs. Document the intent in an issue so reviewers understand the overall roadmap. - ---- - -## 5. Code Review & Merge - -- Respond promptly to reviewer comments. -- Push focused updates; each push re‑runs CI. -- When all reviews are approved and CI is green, a maintainer will merge your PR. - ---- - -## 6. Issue Triage - -If you prefer helping with issue management: -- Comment to clarify reproduction steps. -- Label issues when you have triage rights. -- Close fixed issues and reference the PR. +1. Fork and clone the repository +2. Create a feature branch: `git checkout -b feature/` +3. Make your changes and run `./mfc.sh test` +4. Commit (formatting and linting run automatically via pre-commit hook) +5. Push and open a pull request +See the [developer guide](https://mflowcode.github.io/documentation/md_contributing.html) for coding standards, testing details, and the full PR process. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 7fa0eb6599..f059ecbad0 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -35,30 +35,34 @@ Copilot, when reviewing: | Modules | m_ (e.g. m_transport). | | Public subroutines | s__ (s_compute_flux). | | Public functions | f__. | -| Routine size | subroutine ≤ 500 lines, helper ≤ 150, function ≤ 100, file ≤ 1000. | -| Arguments | ≤ 6; else use a derived-type params struct. | +| Routine size | Prefer subroutine ≤ 500 lines, helper ≤ 150, function ≤ 100, file ≤ 1000. | +| Arguments | Prefer ≤ 6; consider a derived-type params struct for more. | | Forbidden | goto (except legacy), COMMON, save globals. | | Variables | Every arg has explicit intent; use dimension/allocatable/pointer as appropriate. | | Errors | Call s_mpi_abort(), never stop or error stop. | +> **Note:** Size and argument limits are soft guidelines for new code. Existing code may exceed them. Enforce strictly for naming, formatting, forbidden constructs, and error handling. + Copilot, when reviewing: -* Flag violations of any cell above. -* Suggest refactors when size or argument limits are exceeded. +* Flag violations of hard rules (naming, formatting, forbidden constructs, error handling). +* Suggest refactors when size or argument limits are exceeded in new or modified code. * Ensure private helpers stay inside their defining module and avoid nested procedures. --- -## 4 OpenACC Guidelines (for GPU kernels) +## 4 GPU Guidelines + +**Do not use raw OpenACC/OpenMP pragmas.** Use the project's Fypp GPU macros instead: -Wrap tight loops: +* `@:GPU_PARALLEL_LOOP()` for parallel loops +* `@:ALLOCATE()` / `@:DEALLOCATE()` for memory management +* `@:GPU_ENTER_DATA()` / `@:GPU_EXIT_DATA()` for data regions -```fortran -!$acc parallel loop gang vector default(present) reduction(...) -``` +See `src/common/include/macros.fpp` and `src/common/include/parallel_macros.fpp` for the full macro API. +Additional rules: * Add collapse(n) when safe. * Declare loop-local variables with private(...). -* Allocate large arrays with managed or move them into a persistent !$acc enter data region at start-up. * Avoid stop/error stop inside device code. * Code must compile with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index e40ae91b9c..bb457d6be2 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,58 +1,37 @@ ## Description -Please include a summary of the changes and the related issue(s) if they exist. -Please also include relevant motivation and context. +Summarize your changes and the motivation behind them. -Fixes #(issue) [optional] +Fixes #(issue) ### Type of change -Please delete options that are not relevant. +- [ ] Bug fix +- [ ] New feature +- [ ] Refactor +- [ ] Documentation +- [ ] Other: _describe_ -- [ ] Bug fix (non-breaking change which fixes an issue) -- [ ] New feature (non-breaking change which adds functionality) -- [ ] Something else +## Testing -### Scope +Describe how you tested your changes. List the compilers and platforms you used. -- [ ] This PR comprises a set of related changes with a common goal - -If you cannot check the above box, please split your PR into multiple PRs that each have a common goal. - -## How Has This Been Tested? - -Please describe the tests that you ran to verify your changes. -Provide instructions so we can reproduce. -Please also list any relevant details for your test configuration +## Checklist -- [ ] Test A -- [ ] Test B +- [ ] New and existing tests pass locally (`./mfc.sh test`) +- [ ] I added or updated tests for new behavior +- [ ] I updated documentation if user-facing behavior changed -**Test Configuration**: +See the [developer guide](https://mflowcode.github.io/documentation/md_contributing.html) for full coding standards. -* What computers and compilers did you use to test this: +
+GPU changes (expand if you modified code in src/) -## Checklist +- [ ] Code compiles with NVHPC (nvfortran) +- [ ] Code compiles with Cray (ftn) +- [ ] GPU results match CPU results +- [ ] Tested on NVIDIA GPU (V100/A100/H100) or AMD GPU (MI200+) +- [ ] Consider attaching an Nsight Systems or rocprof-systems profile if performance-sensitive +- [ ] Consider testing with multiple GPUs (e.g. 1, 2, 8) to verify scaling -- [ ] I have added comments for the new code -- [ ] I added Doxygen docstrings to the new code -- [ ] I have made corresponding changes to the documentation (`docs/`) -- [ ] I have added regression tests to the test suite so that people can verify in the future that the feature is behaving as expected -- [ ] I have added example cases in `examples/` that demonstrate my new feature performing as expected. -They run to completion and demonstrate "interesting physics" -- [ ] I ran `./mfc.sh format` before committing my code -- [ ] New and existing tests pass locally with my changes, including with GPU capability enabled (both NVIDIA hardware with NVHPC compilers and AMD hardware with CRAY compilers) and disabled -- [ ] This PR does not introduce any repeated code (it follows the [DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself) principle) -- [ ] I cannot think of a way to condense this code and reduce any introduced additional line count - -### If your code changes any code source files (anything in `src/simulation`) - -To make sure the code is performing as expected on GPU devices, I have: -- [ ] Checked that the code compiles using NVHPC compilers -- [ ] Checked that the code compiles using CRAY compilers -- [ ] Ran the code on either V100, A100, or H100 GPUs and ensured the new feature performed as expected (the GPU results match the CPU results) -- [ ] Ran the code on MI200+ GPUs and ensure the new features performed as expected (the GPU results match the CPU results) -- [ ] Enclosed the new feature via `nvtx` ranges so that they can be identified in profiles -- [ ] Ran a Nsight Systems profile using `./mfc.sh run XXXX --gpu -t simulation --nsys`, and have attached the output file (`.nsys-rep`) and plain text results to this PR -- [ ] Ran a Rocprof Systems profile using `./mfc.sh run XXXX --gpu -t simulation --rsys --hip-trace`, and have attached the output file and plain text results to this PR. -- [ ] Ran my code using various numbers of different GPUs (1, 2, and 8, for example) in parallel and made sure that the results scale similarly to what happens if you run without the new code/feature +
diff --git a/.pr_agent.toml b/.pr_agent.toml index 981f9f3044..cdcd9c4ae4 100644 --- a/.pr_agent.toml +++ b/.pr_agent.toml @@ -7,7 +7,9 @@ pr_commands = ["/describe", "/review", "/improve"] num_max_findings = 10 # how many items to surface require_tests_review = true extra_instructions = """ -Focus on duplicate code, the possibility of bugs, and if the PR added appropriate tests if it added a simulation feature. +Follow the coding standards in .github/copilot-instructions.md. +Focus on: duplicate code, potential bugs, missing tests for simulation features, +and GPU compatibility (OpenACC directives, no stop/error stop in device code). """ [pr_code_suggestions] diff --git a/docs/documentation/contributing.md b/docs/documentation/contributing.md new file mode 100644 index 0000000000..540b84682d --- /dev/null +++ b/docs/documentation/contributing.md @@ -0,0 +1,119 @@ +@page contributing Contributing + +# Contributing to MFC + +We welcome contributions of all kinds: bug fixes, new features, documentation, tests, and issue triage. +This guide covers everything you need to get started and get your changes merged. + +## Getting Set Up + +1. **Fork and clone** + ```bash + git clone https://github.com//MFC.git + cd MFC + git remote add upstream https://github.com/MFlowCode/MFC.git + ``` +2. **Build MFC** (see @ref getting-started for full details): + ```bash + ./mfc.sh build -j $(nproc) + ``` +3. **Run the test suite** to verify your environment: + ```bash + ./mfc.sh test -j $(nproc) + ``` + +## Development Workflow + +| Step | Command / Action | +|------|-----------------| +| Sync your fork | `git checkout master && git pull upstream master` | +| Create a branch on your fork | `git checkout -b feature/` | +| Code, test, document | Follow the standards below | +| Run tests | `./mfc.sh test` | +| Commit | Clear, atomic commits (see below) | +| Push to your fork | `git push origin feature/` | +| Open a PR | From your fork to `MFlowCode/MFC:master`. Every push triggers CI -- bundle changes to avoid flooding the queue | + +### Commit Messages + +- Start with a concise (50 chars or fewer) summary in imperative mood: `Fix out-of-bounds in EOS module` +- Add a blank line, then a detailed explanation if needed +- Reference related issues: `Fixes #123` or `Part of #456` + +## Coding Standards + +MFC is written in modern Fortran 2008+ with [Fypp](https://github.com/aradi/fypp) metaprogramming. +The standards below are split into **hard rules** (enforced in CI and review) and **soft guidelines** (goals for new code). + +### Hard Rules + +These are enforced. CI and reviewers will flag violations. + +| Element | Rule | +|---------|------| +| Formatting | Enforced automatically by pre-commit hook (`./mfc.sh format` and `./mfc.sh lint`) | +| Indentation | 2 spaces; continuation lines align beneath `&` | +| Case | Lowercase keywords and intrinsics (`do`, `end subroutine`, ...) | +| Module naming | `m_` (e.g. `m_riemann_solvers`) | +| Public subroutines | `s__` (e.g. `s_compute_flux`) | +| Public functions | `f__` (e.g. `f_create_bbox`) | +| Variables | Every argument has explicit `intent`; use `implicit none` | +| Forbidden | `goto`, `COMMON` blocks, global `save` variables | +| Error handling | Call `s_mpi_abort()` -- never `stop` or `error stop` | +| Compiler support | Code must compile with GNU gfortran, NVIDIA nvfortran, Cray ftn, and Intel ifx | + +### Soft Guidelines + +Aim for these in new and modified code. Existing code may not meet all of them. + +| Element | Guideline | +|---------|-----------| +| File length | Prefer 1000 lines or fewer per file | +| Subroutine length | Prefer 500 lines or fewer (helpers: 150) | +| Function length | Prefer 100 lines or fewer | +| Arguments | Prefer 6 or fewer; consider a derived-type struct for more | +| DRY | Avoid duplicating logic; factor shared code into helpers | + +## Fypp and GPU + +MFC uses Fypp macros (in `src/*/include/`) to generate accelerator-specific Fortran for OpenACC and OpenMP backends. +Key points: + +- Use the project's GPU macros (`@:GPU_PARALLEL_LOOP`, `@:ALLOCATE`, etc.) -- raw OpenACC/OpenMP pragmas are not allowed +- Keep macros simple and readable +- Only `simulation` (plus its `common` dependencies) is GPU-accelerated +- See @ref gpuParallelization for the full macro API reference + +## Testing + +MFC has 500+ regression tests. See @ref testing for the full guide. + +- **Add tests** for any new feature or bug fix +- Use `./mfc.sh test --generate` to create golden files for new cases +- Keep tests fast: use small grids and short runtimes +- Test with `--test-all` to include post-processing validation + +## Documentation + +- Add or update **Doxygen docstrings** in source files for new public routines +- Update **markdown docs** under `docs/` if user-facing behavior changes +- Provide a minimal **example case** in `examples/` for new features when practical + +## Submitting a Pull Request + +1. **PRs come from your fork.** Do not create branches on `MFlowCode/MFC` directly. Push to your fork and open a PR from there against `MFlowCode/MFC:master`. +2. **One PR = one logical change.** Split large changes into focused PRs. +3. **Fill out the PR template.** Remove checklist items that don't apply. +4. **Link issues** with `Fixes #` or `Part of #`. +5. **Ensure CI passes** before requesting review. Run `./mfc.sh test` locally first. Formatting and linting are handled automatically by the pre-commit hook. +6. **Describe your testing**: what you ran, which compilers/platforms you used. + +If your change touches GPU code (`src/simulation/`), see the GPU checklist in the PR template. + +## Code Review and Merge + +- Respond to reviewer comments promptly +- Push focused updates; each push re-runs CI, so batch your fixes +- A maintainer will merge your PR once all reviews are approved and CI is green + +If your PR is large or architectural, consider opening an issue first to discuss the approach. diff --git a/docs/documentation/readme.md b/docs/documentation/readme.md index 3891c363f4..1d70332830 100644 --- a/docs/documentation/readme.md +++ b/docs/documentation/readme.md @@ -27,6 +27,10 @@ Welcome to the Multi-component Flow Code (MFC) documentation. - @ref docker "Containers" - Docker usage - @ref troubleshooting "Troubleshooting" - Debugging and common issues +## Development + +- @ref contributing "Contributing" - Developer guide and coding standards + ## About - @ref papers "Papers" - Publications using MFC From 7364aded46f273de5e85e9f2f227414fac86650a Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Wed, 11 Feb 2026 16:22:46 -0500 Subject: [PATCH 2/7] Add how-to guides and common pitfalls to developer guide, consolidate AI reviewer configs - Add Common Pitfalls section to contributing.md covering array bounds, precision, memory, MPI, physics model consistency, Python toolchain, compiler portability - Add 10 step-by-step How-To Guides: adding parameters, GPU parallel loops, GPU array allocation, test cases, new modules, precision system, MPI halo exchange, post-process variables, src/common/ changes, GPU debugging - Slim copilot-instructions.md to project context + priority list pointing to contributing.md as single source of truth - Update .pr_agent.toml and .coderabbit.yaml to reference all three canonical files Co-Authored-By: Claude Opus 4.6 --- .coderabbit.yaml | 9 +- .github/copilot-instructions.md | 91 ++--- .pr_agent.toml | 16 +- docs/documentation/contributing.md | 548 ++++++++++++++++++++++++++++- 4 files changed, 578 insertions(+), 86 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index b306a9d28f..5ab066e8c3 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -7,12 +7,12 @@ reviews: - path: "src/**/*.fpp" instructions: | Fortran source (Fypp-preprocessed). Follow the coding standards in - .github/copilot-instructions.md. Pay attention to naming conventions, - GPU compatibility (OpenACC), and error handling (s_mpi_abort, not stop). + docs/documentation/contributing.md and the GPU macro API in + docs/documentation/gpuParallelization.md. - path: "src/**/*.f90" instructions: | Fortran source. Follow the coding standards in - .github/copilot-instructions.md. + docs/documentation/contributing.md. - path: "toolchain/**/*.py" instructions: | Python toolchain code. Follow PEP 8. @@ -21,5 +21,6 @@ knowledge_base: code_guidelines: enabled: true filePatterns: - - ".github/copilot-instructions.md" - "docs/documentation/contributing.md" + - "docs/documentation/gpuParallelization.md" + - ".github/copilot-instructions.md" diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index f059ecbad0..137c700b5e 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,79 +1,36 @@ -# GitHub Copilot Instructions – Pull-Request Reviews for MFC +# AI Code Review Instructions for MFC -These instructions guide **GitHub Copilot Code Review** and **Copilot Chat** when they evaluate pull requests in this repository. +These instructions guide AI code reviewers (GitHub Copilot, CodeRabbit, Qodo, Cubic, CodeAnt, etc.) when evaluating pull requests in this repository. ---- - -## 1 Project Context (always include) +**Coding standards, common pitfalls, and contribution guidelines:** `docs/documentation/contributing.md` +**GPU macro API reference:** `docs/documentation/gpuParallelization.md` -* **Project:** MFC (exascale many-physics solver) written in **modern Fortran 2008+**, generated with **Fypp**. -* **Directory layout:** - * Sources in `src/`, tests in `tests/`, examples in `examples/`. - * Most source files are templated `.fpp`; CMake transpiles them to `.f90`. -* **Fypp macros** are in `src//include/`, where `` is `simulation`, `common`, `pre_process`, or `post_process`. Review these first. -* Only `simulation` (plus its `common` dependencies) is GPU-accelerated with **OpenACC**. - -> **Copilot, when reviewing:** -> * Treat the codebase as free-form Fortran 2008+ with `implicit none`, explicit `intent`, and standard intrinsics. -> * Prefer `module … contains … subroutine foo()` over legacy constructs; flag uses of `COMMON`, file-level `include`, or other global state. +Formatting and linting are enforced by pre-commit hooks. Focus review effort on correctness, not style. --- -## 2 Incremental-Change Workflow +## 1 Project Context -Copilot, when reviewing: -* Encourage small, buildable commits +* **Project:** MFC (exascale many-physics solver) written in **modern Fortran 2008+**, preprocessed with **Fypp**. +* **Directory layout:** + * Sources in `src/`, tests in `tests/`, examples in `examples/`, Python toolchain in `toolchain/`. + * Most source files are `.fpp` (Fypp templates); CMake transpiles them to `.f90`. +* **Fypp macros** are in `src//include/`, where `` is `simulation`, `common`, `pre_process`, or `post_process`. +* Only `simulation` (plus its `common` dependencies) is GPU-accelerated via **OpenACC**. +* Code must compile with **GNU gfortran**, **NVIDIA nvfortran**, **Cray ftn**, and **Intel ifx**. +* Precision modes: double (default), single, and mixed (`wp` = working precision, `stp` = storage precision). --- -## 3 Style & Naming Conventions (*.fpp / *.f90) - -| Element | Rule | -|---------|------| -| Indentation | 2 spaces; continuation lines align beneath &. | -| Case | Lower-case keywords & intrinsics (do, end subroutine, …). | -| Modules | m_ (e.g. m_transport). | -| Public subroutines | s__ (s_compute_flux). | -| Public functions | f__. | -| Routine size | Prefer subroutine ≤ 500 lines, helper ≤ 150, function ≤ 100, file ≤ 1000. | -| Arguments | Prefer ≤ 6; consider a derived-type params struct for more. | -| Forbidden | goto (except legacy), COMMON, save globals. | -| Variables | Every arg has explicit intent; use dimension/allocatable/pointer as appropriate. | -| Errors | Call s_mpi_abort(), never stop or error stop. | - -> **Note:** Size and argument limits are soft guidelines for new code. Existing code may exceed them. Enforce strictly for naming, formatting, forbidden constructs, and error handling. - -Copilot, when reviewing: -* Flag violations of hard rules (naming, formatting, forbidden constructs, error handling). -* Suggest refactors when size or argument limits are exceeded in new or modified code. -* Ensure private helpers stay inside their defining module and avoid nested procedures. - ---- - -## 4 GPU Guidelines - -**Do not use raw OpenACC/OpenMP pragmas.** Use the project's Fypp GPU macros instead: - -* `@:GPU_PARALLEL_LOOP()` for parallel loops -* `@:ALLOCATE()` / `@:DEALLOCATE()` for memory management -* `@:GPU_ENTER_DATA()` / `@:GPU_EXIT_DATA()` for data regions - -See `src/common/include/macros.fpp` and `src/common/include/parallel_macros.fpp` for the full macro API. - -Additional rules: -* Add collapse(n) when safe. -* Declare loop-local variables with private(...). -* Avoid stop/error stop inside device code. -* Code must compile with Cray ftn, NVIDIA nvfortran, GNU gfortran, and Intel ifx/ifort. - ---- +## 2 What to Review -## 5 Review Checklist (what Copilot should verify) +See the **Common Pitfalls** section of `docs/documentation/contributing.md` for the full reference. Key review priorities: -1. Buildability: PR includes build instructions or CI passes the staged build. -2. Tests: Focused tests are added/updated. -3. Style compliance: All rules in §3 are satisfied. -4. Module hygiene: No new global state; proper namespacing. -5. Performance: GPU code follows §4; no large host/device transfers in hot loops. -6. Documentation: Updated in-code comments and, when needed, README or docs site. -7. Regressions: No changes to outputs of golden tests without clear justification. +1. **Correctness over style** — logic bugs, numerical issues, array bounds (non-unity lower bounds with ghost cells), domain transposition bugs (Y/Z sweeps reorder indices) +2. **Precision mixing** — `stp` vs `wp` conversions, no double-precision intrinsics (`dsqrt`, `dble`, `real(8)`, etc.) +3. **Memory** — `@:ALLOCATE`/`@:DEALLOCATE` pairing, GPU pointer setup (`@:ACC_SETUP_VFs`/`@:ACC_SETUP_SFs`) +4. **MPI** — halo exchange pack/unpack offsets, `GPU_UPDATE` around MPI calls, buffer sizing +5. **GPU** — no raw OpenACC/OpenMP pragmas (use Fypp GPU macros), `private(...)` on loop-local variables, no `stop`/`error stop` in device code +6. **Physics** — pressure formula must match `model_eqns`, `case_validator.py` constraints for new parameters +7. **Compiler portability** — all four compilers, Fypp macros for GPU and CPU builds +8. **`src/common/` blast radius** — changes affect all three executables diff --git a/.pr_agent.toml b/.pr_agent.toml index cdcd9c4ae4..c9605a01ed 100644 --- a/.pr_agent.toml +++ b/.pr_agent.toml @@ -7,9 +7,19 @@ pr_commands = ["/describe", "/review", "/improve"] num_max_findings = 10 # how many items to surface require_tests_review = true extra_instructions = """ -Follow the coding standards in .github/copilot-instructions.md. -Focus on: duplicate code, potential bugs, missing tests for simulation features, -and GPU compatibility (OpenACC directives, no stop/error stop in device code). +Project context and review priorities: .github/copilot-instructions.md +Coding standards and common pitfalls: docs/documentation/contributing.md +GPU macro API: docs/documentation/gpuParallelization.md + +Prioritize correctness over style (formatting is enforced by pre-commit hooks). +Key areas: logic bugs, numerical issues, +array bounds (non-unity lower bounds with ghost cells), +domain transposition bugs (Y/Z sweeps reorder indices), MPI halo exchange +correctness (pack/unpack offsets, GPU data coherence), precision mixing +(stp vs wp), ALLOCATE/DEALLOCATE pairing (GPU memory leaks), physics model +consistency (pressure formula must match model_eqns), missing case_validator.py +constraints for new parameters, and compiler portability across all four +supported compilers. """ [pr_code_suggestions] diff --git a/docs/documentation/contributing.md b/docs/documentation/contributing.md index 540b84682d..4694546c6e 100644 --- a/docs/documentation/contributing.md +++ b/docs/documentation/contributing.md @@ -54,12 +54,13 @@ These are enforced. CI and reviewers will flag violations. | Formatting | Enforced automatically by pre-commit hook (`./mfc.sh format` and `./mfc.sh lint`) | | Indentation | 2 spaces; continuation lines align beneath `&` | | Case | Lowercase keywords and intrinsics (`do`, `end subroutine`, ...) | -| Module naming | `m_` (e.g. `m_riemann_solvers`) | +| Modules | `m_` (e.g. `m_riemann_solvers`) | | Public subroutines | `s__` (e.g. `s_compute_flux`) | | Public functions | `f__` (e.g. `f_create_bbox`) | -| Variables | Every argument has explicit `intent`; use `implicit none` | +| Variables | Every argument has explicit `intent`; use `implicit none`, `dimension`/`allocatable`/`pointer` as appropriate | | Forbidden | `goto`, `COMMON` blocks, global `save` variables | | Error handling | Call `s_mpi_abort()` -- never `stop` or `error stop` | +| GPU macros | Do not use raw OpenACC/OpenMP pragmas. Use the project's Fypp GPU macros (see below) | | Compiler support | Code must compile with GNU gfortran, NVIDIA nvfortran, Cray ftn, and Intel ifx | ### Soft Guidelines @@ -68,21 +69,544 @@ Aim for these in new and modified code. Existing code may not meet all of them. | Element | Guideline | |---------|-----------| -| File length | Prefer 1000 lines or fewer per file | -| Subroutine length | Prefer 500 lines or fewer (helpers: 150) | -| Function length | Prefer 100 lines or fewer | -| Arguments | Prefer 6 or fewer; consider a derived-type struct for more | +| Routine size | Prefer subroutine ≤ 500 lines, helper ≤ 150, function ≤ 100, file ≤ 1000 | +| Arguments | Prefer ≤ 6; consider a derived-type params struct for more | | DRY | Avoid duplicating logic; factor shared code into helpers | +## Common Pitfalls + +This section documents domain-specific issues that frequently appear in MFC contributions. +Both human reviewers and AI code reviewers reference this section. + +### Array Bounds and Indexing + +- MFC uses **non-unity lower bounds** (e.g., `idwbuff(1)%beg:idwbuff(1)%end` with negative ghost-cell indices). Always verify loop bounds match array declarations. +- **Domain transposition:** In Y/Z sweeps, array index order is transposed (X: `i,j,k`, Y: `j,i,k`, Z: `k,j,i`). Loop bounds must match the transposed ordering. +- **Riemann solver indexing:** Left states at `j`, right states at `j+1`. Off-by-one here corrupts fluxes. + +### Precision and Type Safety + +- **`stp` vs `wp` mixing:** In mixed-precision mode, `stp` (storage) may be half-precision while `wp` (working) is double. Conversions between them must be intentional, especially in MPI pack/unpack and RHS accumulation. +- **No double-precision intrinsics:** `dsqrt`, `dexp`, `dlog`, `dble`, `dabs`, `real(8)`, `real(4)` are forbidden. Use generic intrinsics with `wp` kind. +- **MPI type matching:** `mpi_p` must match `wp`; `mpi_io_p` must match `stp`. Mismatches corrupt communicated data. + +### Memory and Allocation + +- **ALLOCATE/DEALLOCATE pairing:** Every `@:ALLOCATE()` must have a matching `@:DEALLOCATE()`. Missing deallocations leak GPU memory. +- **@:ACC_SETUP_VFs / @:ACC_SETUP_SFs:** Vector/scalar fields must have GPU pointer setup before use in kernels. +- **Conditional allocation:** If an array is allocated inside an `if` block, its deallocation must follow the same condition. +- **Out-of-bounds access:** Fortran is permissive with assumed-shape arrays. Check that index arithmetic stays within declared bounds. + +### MPI Correctness + +- **Halo exchange:** Pack/unpack offset calculations (`pack_offset`, `unpack_offset`) must be correct for both interior and periodic boundaries. Off-by-one causes data corruption. +- **GPU data coherence:** Non-RDMA MPI requires `GPU_UPDATE(host=...)` before send and `GPU_UPDATE(device=...)` after receive. Missing these causes stale data. +- **Buffer sizing:** `halo_size` depends on dimensionality and QBMM state. `v_size` must account for extra bubble variables when QBMM is active. +- **Deadlocks:** Mismatched send/recv counts or tags across MPI ranks. + +### Physics and Model Consistency + +- **Pressure formula** must match `model_eqns` value. Model 2/3 (multi-fluid), model 4 (bubbles), MHD, and hypoelastic each use different EOS formulations. Wrong formula = wrong physics. +- **Conservative-primitive conversion:** Density recovery, kinetic energy, and pressure each have model-specific paths. Verify the correct branch is taken. +- **Volume fractions** must sum to 1. `alpha_rho_K` must be non-negative. Species mass fractions should be clipped to [0,1]. +- **Boundary conditions:** Periodic BCs must match at both ends (`bc_x%beg` and `bc_x%end`). Cylindrical coordinates have special requirements (`bc_y%beg = -14` for axis in 3D). +- **Parameter constraints:** New parameters or physics features must be validated in `toolchain/mfc/case_validator.py`. New features should add corresponding validation. + +### Python Toolchain + +- New parameters in `toolchain/mfc/params/definitions.py` must have correct types, constraints, and tags. +- Validation in `case_validator.py` must cover new interdependencies. +- CLI schema in `toolchain/mfc/cli/commands.py` must match argument parsing. +- Check subprocess calls for shell injection risks and missing error handling. + +### Compiler Portability + +- Any compiler-specific code (`#ifdef __INTEL_COMPILER` etc.) must have fallbacks for all four supported compilers. +- Fypp macros must expand correctly for both GPU and CPU builds (macros are `#ifdef`'d out for non-GPU). +- No hardcoded GPU architectures without CMake detection. + +### Architecture Notes + +- **`src/common/` affects all three executables** (pre_process, simulation, post_process). Changes here have wide blast radius. +- No new global state; private helpers stay inside their defining module. +- Flag modifications to public subroutine signatures, parameter defaults, or output formats. +- Avoid unnecessary host/device transfers in hot loops, redundant allocations, and algorithmic inefficiency. + ## Fypp and GPU -MFC uses Fypp macros (in `src/*/include/`) to generate accelerator-specific Fortran for OpenACC and OpenMP backends. -Key points: +MFC uses [Fypp](https://github.com/aradi/fypp) macros (in `src/*/include/`) to generate accelerator-specific Fortran for OpenACC and OpenMP backends. +Only `simulation` (plus its `common` dependencies) is GPU-accelerated. + +- **Raw OpenACC/OpenMP pragmas are not allowed.** Use the project's Fypp GPU macros instead. +- Add `collapse(n)` when safe, declare loop-local variables with `private(...)`. +- Avoid `stop`/`error stop` inside device code. +- Keep macros simple and readable. + +See @ref gpuParallelization for the full GPU macro API reference, including all parameters, restrictions, examples, and debugging tools. + +## How-To Guides + +Step-by-step recipes for common development tasks. + +### How to Add a New Simulation Parameter + +Adding a parameter touches both the Python toolchain and Fortran source. Follow these steps in order: + +**Step 1: Register in Python** (`toolchain/mfc/params/definitions.py`) + +Add a call to `_r()` inside the `_load()` function: + +```python +_r("my_param", REAL, {"my_feature_tag"}) +``` + +The arguments are: name, type (`INT`, `REAL`, `LOG`, `STR`), and a set of feature tags. You can add an explicit description with `desc="..."`, otherwise one is auto-generated from `_SIMPLE_DESCS` or `_ATTR_DESCS`. + +**Step 2: Add constraints** (same file, `CONSTRAINTS` dict) + +If the parameter has valid ranges or choices: + +```python +CONSTRAINTS = { + # ... + "my_param": {"min": 0, "max": 100}, + # or: "my_param": {"choices": [1, 2, 3]}, +} +``` + +**Step 3: Add dependencies** (same file, `DEPENDENCIES` dict) + +If enabling one parameter requires or recommends others: + +```python +DEPENDENCIES = { + # ... + "my_param": { + "when_true": { + "requires": ["other_param"], + "recommends": ["optional_param"], + } + }, +} +``` + +Triggers include `when_true` (logical is `T`), `when_set` (parameter is not `None`), and `when_value` (parameter equals a specific value). + +**Step 4: Add physics validation** (`toolchain/mfc/case_validator.py`) + +If the parameter has cross-parameter constraints that go beyond simple min/max: + +```python +def check_my_feature(self): + if self.params["my_param"] > 0 and not self.params["other_param"]: + self.errors.append("my_param requires other_param to be set") +``` + +**Step 5: Declare in Fortran** (`src//m_global_parameters.fpp`) + +Add the variable declaration in the appropriate target's global parameters module. Choose the target(s) where the parameter is used: + +- `src/pre_process/m_global_parameters.fpp` +- `src/simulation/m_global_parameters.fpp` +- `src/post_process/m_global_parameters.fpp` + +```fortran +real(wp) :: my_param !< Description of the parameter +``` + +If the parameter is used in GPU kernels, add a GPU declaration: + +```fortran +$:GPU_DECLARE(create='[my_param]') +``` + +**Step 6: Add to Fortran namelist** (`src//m_start_up.fpp`) + +Add the parameter name to the `namelist /user_inputs/` declaration: + +```fortran +namelist /user_inputs/ ... , my_param, ... +``` + +The toolchain writes the parameter to the input file and Fortran reads it via this namelist. No other I/O code is needed. + +**Step 7: Use in Fortran code** + +Reference `my_param` anywhere in the target's modules. It is available as a global after the namelist is read at startup. + +### How to Write a GPU Parallel Loop + +All GPU loops use Fypp macros. See @ref gpuParallelization for the full API. + +**Simple parallel loop** (3D with collapse): + +```fortran +$:GPU_PARALLEL_LOOP(collapse=3) +do l = 0, p + do k = 0, n + do j = 0, m + q_sf(j, k, l) = 0._wp + end do + end do +end do +$:END_GPU_PARALLEL_LOOP() +``` + +**With private variables** (temporaries local to each thread): + +```fortran +$:GPU_PARALLEL_LOOP(collapse=3, private='[rho, pres, vel]') +do l = 0, p + do k = 0, n + do j = 0, m + rho = q_prim_vf(1)%sf(j, k, l) + pres = q_prim_vf(E_idx)%sf(j, k, l) + ! ... use rho, pres as thread-local ... + end do + end do +end do +$:END_GPU_PARALLEL_LOOP() +``` + +**With reduction:** + +```fortran +$:GPU_PARALLEL_LOOP(collapse=3, & + & reduction='[[my_sum], [my_max]]', & + & reductionOp='[+, MAX]', & + & copy='[my_sum, my_max]') +do l = 0, p + do k = 0, n + do j = 0, m + my_sum = my_sum + q_sf(j, k, l) + my_max = max(my_max, q_sf(j, k, l)) + end do + end do +end do +$:END_GPU_PARALLEL_LOOP() +``` + +**Sequential inner loop** within a parallel region: + +```fortran +$:GPU_PARALLEL_LOOP(collapse=3) +do l = 0, p + do k = 0, n + do j = 0, m + $:GPU_LOOP(parallelism='[seq]') + do i = 1, num_fluids + alpha(i) = q_prim_vf(advxb + i - 1)%sf(j, k, l) + end do + end do + end do +end do +$:END_GPU_PARALLEL_LOOP() +``` + +Key rules: +- Always pair `$:GPU_PARALLEL_LOOP(...)` with `$:END_GPU_PARALLEL_LOOP()` +- Use `collapse(n)` to fuse nested loops when the loop bounds are independent +- Declare all loop-local temporaries in `private='[...]'` +- Never use `stop` or `error stop` inside a GPU loop + +### How to Allocate and Manage GPU Arrays + +The full lifecycle of a GPU-resident array: + +**Step 1: Declare** with GPU directive for module-level variables: + +```fortran +real(wp), allocatable, dimension(:,:,:) :: my_array +$:GPU_DECLARE(create='[my_array]') +``` + +**Step 2: Allocate** in your initialization subroutine: + +```fortran +@:ALLOCATE(my_array(0:m, 0:n, 0:p)) +``` + +`@:ALLOCATE` handles both the Fortran `allocate` and the GPU `enter data create`. + +**Step 3: Setup pointer fields** (only needed for derived types with pointer components like `scalar_field`): + +```fortran +@:ALLOCATE(my_field%sf(0:m, 0:n, 0:p)) +@:ACC_SETUP_SFs(my_field) +``` + +`@:ACC_SETUP_SFs` registers the pointer with the GPU runtime (required on Cray). + +**Step 4: Deallocate** in your finalization subroutine, mirroring every allocation: + +```fortran +@:DEALLOCATE(my_array) +``` + +If an array is allocated inside an `if` block, its deallocation must follow the same condition. + +### How to Add a Test Case + +**Step 1: Create a case file** + +Test cases are Python scripts that print a JSON dict of parameters. See `examples/` for templates: + +```python +#!/usr/bin/env python3 +import json + +print(json.dumps({ + "run_time_info": "F", + "x_domain%beg": 0.0, + "x_domain%end": 1.0, + "m": 49, + "n": 0, + "p": 0, + "dt": 1e-6, + "t_step_start": 0, + "t_step_stop": 100, + "t_step_save": 100, + "num_patches": 1, + "model_eqns": 2, + "num_fluids": 1, + "time_stepper": 3, + "weno_order": 5, + "riemann_solver": 1, + "patch_icpp(1)%geometry": 1, + "patch_icpp(1)%x_centroid": 0.5, + "patch_icpp(1)%length_x": 1.0, + "patch_icpp(1)%vel(1)": 0.0, + "patch_icpp(1)%pres": 1.0, + "patch_icpp(1)%alpha_rho(1)": 1.0, + "patch_icpp(1)%alpha(1)": 1.0, + "fluid_pp(1)%gamma": 0.4, + "fluid_pp(1)%pi_inf": 0.0, +})) +``` + +Keep grids small and runtimes short. + +**Step 2: Register as a regression test** (`toolchain/mfc/test/cases.py`) + +Add your case using the `Case` dataclass and the stack pattern for parameterized variations: + +```python +stack.push("my_feature", {"my_param": value}) +cases.append(define_case_d(stack, '', {})) +stack.pop() +``` + +**Step 3: Generate golden files** + +```bash +./mfc.sh test --generate -o +``` + +Golden files are stored as binary snapshots in `tests//`. + +**Step 4: Run** + +```bash +./mfc.sh test -j $(nproc) +``` + +### How to Create a New Fortran Module + +**Step 1: Create the file** + +Name it `src//m_.fpp`. CMake auto-discovers `.fpp` files — no build system changes needed. + +**Step 2: Use this boilerplate:** + +```fortran +!> @file m_my_feature.fpp +!! @brief Description of the module + +#:include 'case.fpp' +#:include 'macros.fpp' + +module m_my_feature + + use m_derived_types + use m_global_parameters + use m_mpi_proxy + + implicit none + + private; public :: s_initialize_my_feature, & + s_compute_my_feature, & + s_finalize_my_feature + + ! Module-level data + real(wp), allocatable, dimension(:,:,:) :: work_array + +contains + + !> Initialize module data + impure subroutine s_initialize_my_feature() + @:ALLOCATE(work_array(0:m, 0:n, 0:p)) + end subroutine s_initialize_my_feature + + !> Core computation + subroutine s_compute_my_feature(q_prim_vf, rhs_vf) + type(scalar_field), dimension(sys_size), intent(in) :: q_prim_vf + type(scalar_field), dimension(sys_size), intent(inout) :: rhs_vf + ! ... + end subroutine s_compute_my_feature + + !> Clean up module data + impure subroutine s_finalize_my_feature() + @:DEALLOCATE(work_array) + end subroutine s_finalize_my_feature + +end module m_my_feature +``` + +Key conventions: +- `private` by default, explicitly `public` for the module API +- Initialize/finalize subroutines for allocation lifecycle +- Every `@:ALLOCATE` has a matching `@:DEALLOCATE` +- Every argument has explicit `intent` + +### Working with the Precision System + +MFC supports double (default), single, and mixed precision. The types are defined in `src/common/m_precision_select.f90`: + +| Type | Purpose | Example | +|------|---------|---------| +| `wp` | Working precision (computation) | `real(wp) :: velocity` | +| `stp` | Storage precision (I/O, field storage) | `real(stp), pointer :: sf(:,:,:)` | +| `mpi_p` | MPI type matching `wp` | `call MPI_BCAST(var, 1, mpi_p, ...)` | +| `mpi_io_p` | MPI type matching `stp` | Used in parallel I/O | + +Rules: +- Use `real(wp)` for all computational variables +- Literal constants need the `_wp` suffix: `1.0_wp`, `3.14159_wp`, `1e-6_wp` +- Use **generic** intrinsics only: `sqrt`, `abs`, `sin`, `exp`, `log`, `max`, `min` +- **Forbidden** double-precision intrinsics: `dsqrt`, `dexp`, `dlog`, `dble`, `dabs`, `real(8)`, `real(4)` +- Conversions between `stp` and `wp` must be intentional, especially in MPI pack/unpack + +### How to Extend MPI Halo Exchange + +Halo exchange is in `src/simulation/m_mpi_proxy.fpp` (and `src/common/m_mpi_common.fpp` for buffer allocation). + +To add new data to the halo exchange: + +**Step 1: Update buffer sizing** (`src/common/m_mpi_common.fpp`) + +`v_size` determines how many variables are packed per cell. If your new data adds fields per cell, increase `v_size`: + +```fortran +v_size = sys_size + my_extra_fields +``` + +**Step 2: Add pack loop** (`src/simulation/m_mpi_proxy.fpp`) + +Pack your data into the send buffer using a linear index: + +```fortran +$:GPU_PARALLEL_LOOP(collapse=3, private='[j,k,l,r]') +do l = 0, p + do k = 0, n + do j = 0, buff_size - 1 + r = j + buff_size*(k + (n + 1)*l) + buff_send(r) = my_data%sf(j + pack_offset, k, l) + end do + end do +end do +$:END_GPU_PARALLEL_LOOP() +``` + +**Step 3: GPU data coherence** + +For non-RDMA MPI, add host/device transfers around the MPI call: + +```fortran +$:GPU_UPDATE(host='[buff_send]') ! GPU → CPU before send +call MPI_SENDRECV(buff_send, ..., buff_recv, ..., ierr) +$:GPU_UPDATE(device='[buff_recv]') ! CPU → GPU after receive +``` + +**Step 4: Add unpack loop** mirroring the pack loop with `unpack_offset`. + +### How to Add a Post-Processing Output Variable + +Post-processing derived variables live in `src/post_process/m_derived_variables.fpp`. + +**Step 1: Allocate storage** in `s_initialize_derived_variables_module`: + +```fortran +if (my_var_wrt) then + allocate(my_var_sf(-offset_x%beg:m + offset_x%end, & + -offset_y%beg:n + offset_y%end, & + -offset_z%beg:p + offset_z%end)) +end if +``` + +**Step 2: Create derivation subroutine:** + +```fortran +subroutine s_derive_my_variable(q_prim_vf, q_sf) + type(scalar_field), dimension(sys_size), intent(in) :: q_prim_vf + real(wp), dimension(-offset_x%beg:m + offset_x%end, & + -offset_y%beg:n + offset_y%end, & + -offset_z%beg:p + offset_z%end), & + intent(inout) :: q_sf + integer :: i, j, k + + do k = -offset_z%beg, p + offset_z%end + do j = -offset_y%beg, n + offset_y%end + do i = -offset_x%beg, m + offset_x%end + q_sf(i, j, k) = ! ... compute from q_prim_vf ... + end do + end do + end do +end subroutine s_derive_my_variable +``` + +**Step 3: Call from output** in `m_data_output.fpp`: + +```fortran +if (my_var_wrt) then + call s_derive_my_variable(q_prim_vf, q_sf) + call s_write_variable_to_formatted_database_file(q_sf, 'my_variable', dbfile, dbroot) +end if +``` + +### Modifying `src/common/` + +Code in `src/common/` is compiled into all three executables (pre_process, simulation, post_process). Changes here have wide blast radius. + +Checklist: +- Test all three targets: `./mfc.sh test` covers this +- If adding GPU code, remember that only `simulation` is GPU-accelerated. Guard GPU macros with `#:if MFC_SIMULATION` +- Check that new `use` statements don't create circular dependencies +- New modules need `implicit none` and explicit `intent` on all arguments + +### Debugging on GPU + +Set environment variables before running to get diagnostic output: + +**NVIDIA (nvfortran):** + +| Variable | Effect | +|----------|--------| +| `NVCOMPILER_ACC_TIME=1` | Print kernel timing summary | +| `NVCOMPILER_ACC_NOTIFY=1` | Log kernel launches (2 = + data copies, 3 = both) | + +**Cray (ftn):** + +| Variable | Effect | +|----------|--------| +| `CRAY_ACC_DEBUG=1` | Runtime debug logging (0-3, higher = more verbose) | + +**OpenMP offload:** + +| Variable | Effect | +|----------|--------| +| `OMP_TARGET_OFFLOAD=DISABLED` | Run on CPU only (useful for isolating GPU bugs) | -- Use the project's GPU macros (`@:GPU_PARALLEL_LOOP`, `@:ALLOCATE`, etc.) -- raw OpenACC/OpenMP pragmas are not allowed -- Keep macros simple and readable -- Only `simulation` (plus its `common` dependencies) is GPU-accelerated -- See @ref gpuParallelization for the full macro API reference +See @ref gpuParallelization for the full debugging reference. ## Testing From 2a3e94ff5b2540364dd6e9f1cc7f8742de8b9d8a Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Wed, 11 Feb 2026 16:31:57 -0500 Subject: [PATCH 3/7] Fix test flag: use -a instead of --test-all for post-processing validation Co-Authored-By: Claude Opus 4.6 --- docs/documentation/contributing.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/documentation/contributing.md b/docs/documentation/contributing.md index 4694546c6e..2413ec8244 100644 --- a/docs/documentation/contributing.md +++ b/docs/documentation/contributing.md @@ -615,7 +615,7 @@ MFC has 500+ regression tests. See @ref testing for the full guide. - **Add tests** for any new feature or bug fix - Use `./mfc.sh test --generate` to create golden files for new cases - Keep tests fast: use small grids and short runtimes -- Test with `--test-all` to include post-processing validation +- Test with `-a` to include post-processing validation ## Documentation From 11dd7031dc4f12892533f64ccb6a8eb32f866e21 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Wed, 11 Feb 2026 17:01:37 -0500 Subject: [PATCH 4/7] Add architecture overview, CI pipeline guide, and AI reviewer PR-pattern triggers - Add Architecture Overview section: three-phase pipeline, directory layout, simulation data flow, key data structures, build toolchain - Add CI Pipeline section: lint gate checks, build/test matrix, HPC runners, common CI failures and fixes table - Add PR-pattern triggers to copilot-instructions.md for AI reviewers to flag common mistakes (missing parameter pipeline steps, ALLOCATE/DEALLOCATE pairing, stale doc references, missing tests) - Replace inline GPU debugging tables with link to troubleshooting page - Add cross-references to parameters, case_constraints, and troubleshooting pages - Fix CONTRIBUTING.md docs site URL - Remove overly specific domain transposition guidance Co-Authored-By: Claude Opus 4.6 --- .github/CONTRIBUTING.md | 4 +- .github/copilot-instructions.md | 17 +++- .pr_agent.toml | 3 +- docs/documentation/contributing.md | 120 +++++++++++++++++++++++------ 4 files changed, 115 insertions(+), 29 deletions(-) diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index c0c45a1470..05147a4144 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -2,7 +2,7 @@ We welcome contributions of all kinds -- bug fixes, new features, documentation, tests, and issue triage. -**Full developer guide:** [mflowcode.github.io/documentation/md_contributing.html](https://mflowcode.github.io/documentation/md_contributing.html) +**Full developer guide:** [mflowcode.github.io/documentation/md_contributing.html](https://mflowcode.github.io/documentation/contributing.html) ## Quick Reference @@ -12,4 +12,4 @@ We welcome contributions of all kinds -- bug fixes, new features, documentation, 4. Commit (formatting and linting run automatically via pre-commit hook) 5. Push and open a pull request -See the [developer guide](https://mflowcode.github.io/documentation/md_contributing.html) for coding standards, testing details, and the full PR process. +See the [developer guide](https://mflowcode.github.io/documentation/contributing.html) for coding standards, testing details, and the full PR process. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 137c700b5e..6ba315ca62 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -26,7 +26,7 @@ Formatting and linting are enforced by pre-commit hooks. Focus review effort on See the **Common Pitfalls** section of `docs/documentation/contributing.md` for the full reference. Key review priorities: -1. **Correctness over style** — logic bugs, numerical issues, array bounds (non-unity lower bounds with ghost cells), domain transposition bugs (Y/Z sweeps reorder indices) +1. **Correctness over style** — logic bugs, numerical issues, array bounds (non-unity lower bounds with ghost cells) 2. **Precision mixing** — `stp` vs `wp` conversions, no double-precision intrinsics (`dsqrt`, `dble`, `real(8)`, etc.) 3. **Memory** — `@:ALLOCATE`/`@:DEALLOCATE` pairing, GPU pointer setup (`@:ACC_SETUP_VFs`/`@:ACC_SETUP_SFs`) 4. **MPI** — halo exchange pack/unpack offsets, `GPU_UPDATE` around MPI calls, buffer sizing @@ -34,3 +34,18 @@ See the **Common Pitfalls** section of `docs/documentation/contributing.md` for 6. **Physics** — pressure formula must match `model_eqns`, `case_validator.py` constraints for new parameters 7. **Compiler portability** — all four compilers, Fypp macros for GPU and CPU builds 8. **`src/common/` blast radius** — changes affect all three executables + +--- + +## 3 PR-Pattern Triggers + +Flag these patterns when reviewing a pull request: + +* PR adds a parameter in `toolchain/mfc/params/definitions.py` but does not update the `namelist /user_inputs/` in `src/*/m_start_up.fpp` or declare it in `src/*/m_global_parameters.fpp` +* PR adds a parameter with cross-parameter constraints but does not add validation in `toolchain/mfc/case_validator.py` +* PR modifies files in `src/common/` but does not mention testing all three targets (pre_process, simulation, post_process) +* PR adds `@:ALLOCATE` calls without matching `@:DEALLOCATE` in the corresponding finalization subroutine +* PR renames or moves files referenced in `docs/documentation/contributing.md` or `docs/documentation/gpuParallelization.md` without updating those docs +* PR adds a new `.fpp` module missing `implicit none` or missing `private`/`public` declarations +* PR modifies MPI pack/unpack logic in one sweep direction without updating all directions +* PR adds a new physics feature or model without a corresponding regression test diff --git a/.pr_agent.toml b/.pr_agent.toml index c9605a01ed..594b6629ae 100644 --- a/.pr_agent.toml +++ b/.pr_agent.toml @@ -13,8 +13,7 @@ GPU macro API: docs/documentation/gpuParallelization.md Prioritize correctness over style (formatting is enforced by pre-commit hooks). Key areas: logic bugs, numerical issues, -array bounds (non-unity lower bounds with ghost cells), -domain transposition bugs (Y/Z sweeps reorder indices), MPI halo exchange +array bounds (non-unity lower bounds with ghost cells), MPI halo exchange correctness (pack/unpack offsets, GPU data coherence), precision mixing (stp vs wp), ALLOCATE/DEALLOCATE pairing (GPU memory leaks), physics model consistency (pressure formula must match model_eqns), missing case_validator.py diff --git a/docs/documentation/contributing.md b/docs/documentation/contributing.md index 2413ec8244..0f2cbf9db7 100644 --- a/docs/documentation/contributing.md +++ b/docs/documentation/contributing.md @@ -22,6 +22,62 @@ This guide covers everything you need to get started and get your changes merged ./mfc.sh test -j $(nproc) ``` +## Architecture Overview + +Understanding MFC's structure helps you know where to make changes and what they affect. + +### Three-Phase Pipeline + +MFC runs simulations in three phases, each a separate Fortran executable: + +1. **pre_process** — Reads case parameters, generates initial conditions (patch geometries, flow states), writes binary grid and flow data to disk. +2. **simulation** — Reads the initial state, advances the solution in time via TVD Runge-Kutta integration, writes solution snapshots at specified intervals. +3. **post_process** — Reads simulation snapshots, computes derived quantities (vorticity, Schlieren, sound speed, etc.), writes Silo/HDF5 output for visualization. + +All three share code in `src/common/`. Only `simulation` is GPU-accelerated. + +### Directory Layout + +| Directory | Contents | +|-----------|----------| +| `src/simulation/` | Time-stepping, RHS, Riemann solvers, WENO, physics models (GPU-accelerated) | +| `src/pre_process/` | Initial condition generation | +| `src/post_process/` | Derived variable computation and formatted output | +| `src/common/` | Derived types, global parameters, MPI, precision, I/O — shared by all three executables | +| `toolchain/` | Python CLI, parameter system, case validation, build orchestration | +| `tests/` | Golden files for 500+ regression tests | +| `examples/` | Sample case files | +| `docs/` | Doxygen documentation source | + +### Simulation Data Flow + +Each time step, `simulation` computes the right-hand side through this pipeline: + +``` +q_cons_vf (conservative variables: density, momentum, energy, volume fractions) + → convert to primitive (density, velocity, pressure) + → WENO reconstruction (left/right states at cell faces) + → Riemann solve (numerical fluxes) + → flux divergence + source terms (viscous, surface tension, body forces) + → RHS assembly + → Runge-Kutta update → q_cons_vf (next stage/step) +``` + +Key data structures (defined in `src/common/m_derived_types.fpp`): +- `scalar_field` — wraps a 3D `real(stp)` array (`%sf(i,j,k)`) +- `vector_field` — array of `scalar_field` (`%vf(1:sys_size)`) +- `q_cons_vf` / `q_prim_vf` — conservative and primitive state vectors + +### Build Toolchain + +`./mfc.sh` is a shell wrapper that invokes the Python toolchain (`toolchain/main.py`), which orchestrates: + +1. **CMake** configures the build (compiler detection, dependencies, GPU backend) +2. **Fypp** preprocesses `.fpp` files into `.f90` (expands GPU macros, code generation) +3. **Fortran compiler** builds three executables from the generated `.f90` files + +See @ref parameters for the full list of ~3,400 simulation parameters. See @ref case_constraints for feature compatibility and example configurations. + ## Development Workflow | Step | Command / Action | @@ -81,7 +137,6 @@ Both human reviewers and AI code reviewers reference this section. ### Array Bounds and Indexing - MFC uses **non-unity lower bounds** (e.g., `idwbuff(1)%beg:idwbuff(1)%end` with negative ghost-cell indices). Always verify loop bounds match array declarations. -- **Domain transposition:** In Y/Z sweeps, array index order is transposed (X: `i,j,k`, Y: `j,i,k`, Z: `k,j,i`). Loop bounds must match the transposed ordering. - **Riemann solver indexing:** Left states at `j`, right states at `j+1`. Off-by-one here corrupts fluxes. ### Precision and Type Safety @@ -150,7 +205,7 @@ Step-by-step recipes for common development tasks. ### How to Add a New Simulation Parameter -Adding a parameter touches both the Python toolchain and Fortran source. Follow these steps in order: +Adding a parameter touches both the Python toolchain and Fortran source. Follow these steps in order. See @ref parameters for the full list of existing parameters and @ref case_constraints for feature compatibility. **Step 1: Register in Python** (`toolchain/mfc/params/definitions.py`) @@ -583,39 +638,56 @@ Checklist: - Check that new `use` statements don't create circular dependencies - New modules need `implicit none` and explicit `intent` on all arguments -### Debugging on GPU +### Debugging -Set environment variables before running to get diagnostic output: +See @ref troubleshooting for debugging workflows, profiling tools, GPU diagnostic environment variables, common build/runtime errors, and fixes. -**NVIDIA (nvfortran):** +## Testing -| Variable | Effect | -|----------|--------| -| `NVCOMPILER_ACC_TIME=1` | Print kernel timing summary | -| `NVCOMPILER_ACC_NOTIFY=1` | Log kernel launches (2 = + data copies, 3 = both) | +MFC has 500+ regression tests. See @ref testing for the full guide. -**Cray (ftn):** +- **Add tests** for any new feature or bug fix +- Use `./mfc.sh test --generate` to create golden files for new cases +- Keep tests fast: use small grids and short runtimes +- Test with `-a` to include post-processing validation -| Variable | Effect | -|----------|--------| -| `CRAY_ACC_DEBUG=1` | Runtime debug logging (0-3, higher = more verbose) | +## CI Pipeline -**OpenMP offload:** +Every push to a PR triggers CI. Understanding the pipeline helps you fix failures quickly. -| Variable | Effect | -|----------|--------| -| `OMP_TARGET_OFFLOAD=DISABLED` | Run on CPU only (useful for isolating GPU bugs) | +### Lint Gate (runs first, blocks all other jobs) -See @ref gpuParallelization for the full debugging reference. +All four checks must pass before any builds start: -## Testing +1. **Formatting** — `./mfc.sh format` (auto-handled by pre-commit hook) +2. **Spelling** — `./mfc.sh spelling` +3. **Toolchain lint** — `./mfc.sh lint` (Python code quality) +4. **Source lint** — checks for: + - Raw `!$acc` or `!$omp` directives (must use Fypp GPU macros) + - Double-precision intrinsics (`dsqrt`, `dexp`, `dble`, etc.) -MFC has 500+ regression tests. See @ref testing for the full guide. +### Build and Test Matrix -- **Add tests** for any new feature or bug fix -- Use `./mfc.sh test --generate` to create golden files for new cases -- Keep tests fast: use small grids and short runtimes -- Test with `-a` to include post-processing validation +After the lint gate passes: + +- **Platforms:** Ubuntu and macOS +- **Compilers:** GNU (both), Intel OneAPI (Ubuntu only) +- **Modes:** debug + release, MPI + no-MPI, double + single precision +- **HPC runners:** Phoenix (NVIDIA/nvfortran), Frontier (AMD/Cray ftn) — both OpenACC and OpenMP backends +- **Retries:** Tests retry up to 3 times before failing +- **Cleanliness check:** Compiler warnings are tracked — your PR cannot increase the warning count + +### Common CI Failures + +| Failure | Fix | +|---------|-----| +| Formatting check | Pre-commit hook handles this; if you bypassed it, run `./mfc.sh format` | +| Raw pragma detected | Replace `!$acc`/`!$omp` with Fypp GPU macros (see @ref gpuParallelization) | +| Double-precision intrinsic | Use generic intrinsic with `wp` kind (e.g., `sqrt` not `dsqrt`) | +| Golden file mismatch | If intentional: `./mfc.sh test --generate --only ` | +| Warnings increased | Fix the new compiler warnings before merging | + +See @ref troubleshooting for detailed debugging workflows. ## Documentation From 50d372adaf65d9ad91db0195ab929e75226bbdf7 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Wed, 11 Feb 2026 17:08:24 -0500 Subject: [PATCH 5/7] Add doc reference checker to pre-commit lint pipeline - New toolchain/mfc/lint_docs.py: extracts file paths from documentation markdown files and verifies they exist in the repo - Integrated as step 5/5 in precheck.sh (runs on every commit) - Scans contributing.md, gpuParallelization.md, running.md, case.md, copilot-instructions.md, .pr_agent.toml, and .coderabbit.yaml - Fix stale file paths in case.md: hardcoded IC files are in src/common/include/, not src/pre_process/include/ Co-Authored-By: Claude Opus 4.6 --- docs/documentation/case.md | 6 ++-- toolchain/bootstrap/precheck.sh | 16 +++++++--- toolchain/mfc/lint_docs.py | 54 +++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 7 deletions(-) create mode 100644 toolchain/mfc/lint_docs.py diff --git a/docs/documentation/case.md b/docs/documentation/case.md index 90e4e5e147..f9073406d1 100644 --- a/docs/documentation/case.md +++ b/docs/documentation/case.md @@ -236,9 +236,9 @@ end if Some patch configurations are not adequately handled with the above analytic variable definitions. In this case, a hard coded patch can be used. -Hard coded patches can be added by adding additional hard coded patch identifiers to `src/pre_process/include/1[2,3]dHardcodedIC.fpp`. +Hard coded patches can be added by adding additional hard coded patch identifiers to `src/common/include/1[2,3]dHardcodedIC.fpp`. When using a hard coded patch, the `patch_icpp(patch_id)%%hcid` must be set to the hard-coded patch id. -For example, to add a 2D Hardcoded patch with an id of 200, one would add the following to `src/pre_process/include/2dHardcodedIC.fpp` +For example, to add a 2D Hardcoded patch with an id of 200, one would add the following to `src/common/include/2dHardcodedIC.fpp` ```f90 case(200) @@ -256,7 +256,7 @@ The code provides three pre-built patches for dimensional extrusion of initial c - `case(370)`: Extrude 2D data to 3D domain Setup: Only requires specifying `init_dir` and filename pattern via `zeros_default`. Grid dimensions are automatically detected from the data files. -Implementation: All variables and file handling are managed in `src/pre_process/include/ExtrusionHardcodedIC.fpp` with no manual grid configuration needed. +Implementation: All variables and file handling are managed in `src/common/include/ExtrusionHardcodedIC.fpp` with no manual grid configuration needed. Usage: Ideal for initializing simulations from lower-dimensional solutions, enabling users to add perturbations or modifications to the base extruded fields for flow instability studies. #### Parameter Descriptions diff --git a/toolchain/bootstrap/precheck.sh b/toolchain/bootstrap/precheck.sh index e215b5945e..41f0159294 100755 --- a/toolchain/bootstrap/precheck.sh +++ b/toolchain/bootstrap/precheck.sh @@ -61,7 +61,7 @@ log "Running$MAGENTA precheck$COLOR_RESET (same checks as CI lint-gate)..." echo "" # 1. Check formatting -log "[$CYAN 1/4$COLOR_RESET] Checking$MAGENTA formatting$COLOR_RESET..." +log "[$CYAN 1/5$COLOR_RESET] Checking$MAGENTA formatting$COLOR_RESET..." # Capture state before formatting BEFORE_HASH=$(git diff -- '*.f90' '*.fpp' '*.py' 2>/dev/null | compute_hash) if ! ./mfc.sh format -j "$JOBS" > /dev/null 2>&1; then @@ -82,7 +82,7 @@ else fi # 2. Spell check -log "[$CYAN 2/4$COLOR_RESET] Running$MAGENTA spell check$COLOR_RESET..." +log "[$CYAN 2/5$COLOR_RESET] Running$MAGENTA spell check$COLOR_RESET..." if ./mfc.sh spelling > /dev/null 2>&1; then ok "Spell check passed." else @@ -91,7 +91,7 @@ else fi # 3. Lint toolchain (Python) -log "[$CYAN 3/4$COLOR_RESET] Running$MAGENTA toolchain lint$COLOR_RESET..." +log "[$CYAN 3/5$COLOR_RESET] Running$MAGENTA toolchain lint$COLOR_RESET..." if ./mfc.sh lint > /dev/null 2>&1; then ok "Toolchain lint passed." else @@ -100,7 +100,7 @@ else fi # 4. Source code lint checks -log "[$CYAN 4/4$COLOR_RESET] Running$MAGENTA source lint$COLOR_RESET checks..." +log "[$CYAN 4/5$COLOR_RESET] Running$MAGENTA source lint$COLOR_RESET checks..." SOURCE_FAILED=0 # Check for raw OpenACC/OpenMP directives @@ -127,6 +127,14 @@ else FAILED=1 fi +# 5. Doc reference check +log "[$CYAN 5/5$COLOR_RESET] Checking$MAGENTA doc references$COLOR_RESET..." +if python3 toolchain/mfc/lint_docs.py 2>&1; then + ok "Doc references are valid." +else + FAILED=1 +fi + echo "" if [ $FAILED -eq 0 ]; then diff --git a/toolchain/mfc/lint_docs.py b/toolchain/mfc/lint_docs.py new file mode 100644 index 0000000000..565b3f2332 --- /dev/null +++ b/toolchain/mfc/lint_docs.py @@ -0,0 +1,54 @@ +"""Check that file paths referenced in documentation still exist.""" + +import re +import sys +from pathlib import Path + +# Docs to scan for file path references +DOCS = [ + "docs/documentation/contributing.md", + "docs/documentation/gpuParallelization.md", + "docs/documentation/running.md", + "docs/documentation/case.md", + ".github/copilot-instructions.md", + ".pr_agent.toml", + ".coderabbit.yaml", +] + +# Match backtick-wrapped strings that look like repo-relative file paths +PATH_RE = re.compile(r"`((?:src|toolchain|\.github|docs|examples|tests)/[^`]+)`") + +# Skip paths with placeholders, globs, or patterns (not real file paths) +SKIP_RE = re.compile(r"[<>*()\[\]{}]|/\.\.\.|%|\$") + + +def check_docs(repo_root: Path) -> list[str]: + errors = [] + for doc in DOCS: + doc_path = repo_root / doc + if not doc_path.exists(): + continue + text = doc_path.read_text() + for match in PATH_RE.finditer(text): + path_str = match.group(1) + if SKIP_RE.search(path_str): + continue + # Strip trailing punctuation that may have leaked in + path_str = path_str.rstrip(".,;:!?") + if not (repo_root / path_str).exists(): + errors.append(f" {doc} references '{path_str}' but it does not exist") + return errors + + +def main(): + repo_root = Path(__file__).resolve().parents[2] + errors = check_docs(repo_root) + if errors: + print("Doc reference check failed:") + for e in errors: + print(e) + sys.exit(1) + + +if __name__ == "__main__": + main() From f2774562fa352ddd5e738f294c908d5dfc92a8bd Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Wed, 11 Feb 2026 17:26:26 -0500 Subject: [PATCH 6/7] Streamline PR template: remove CI-redundant checks, fix URL - Remove "tests pass locally" checklist item (CI enforces this) - Remove compiler checklist items from GPU section (CI tests these) - Remove "Consider" profiling/scaling lines (noise for most PRs) - Narrow GPU section trigger to src/simulation/ (only GPU-accelerated target) - Simplify testing prompt to open-ended question - Fix developer guide URL Co-Authored-By: Claude Opus 4.6 --- .github/pull_request_template.md | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index bb457d6be2..2f02be01f6 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -14,24 +14,19 @@ Fixes #(issue) ## Testing -Describe how you tested your changes. List the compilers and platforms you used. +How did you test your changes? ## Checklist -- [ ] New and existing tests pass locally (`./mfc.sh test`) - [ ] I added or updated tests for new behavior - [ ] I updated documentation if user-facing behavior changed -See the [developer guide](https://mflowcode.github.io/documentation/md_contributing.html) for full coding standards. +See the [developer guide](https://mflowcode.github.io/documentation/contributing.html) for full coding standards.
-GPU changes (expand if you modified code in src/) +GPU changes (expand if you modified src/simulation/) -- [ ] Code compiles with NVHPC (nvfortran) -- [ ] Code compiles with Cray (ftn) - [ ] GPU results match CPU results -- [ ] Tested on NVIDIA GPU (V100/A100/H100) or AMD GPU (MI200+) -- [ ] Consider attaching an Nsight Systems or rocprof-systems profile if performance-sensitive -- [ ] Consider testing with multiple GPUs (e.g. 1, 2, 8) to verify scaling +- [ ] Tested on NVIDIA GPU or AMD GPU
From f6e5829fc5dc176810008ae8b63bfe07b0aa6189 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Wed, 11 Feb 2026 17:48:31 -0500 Subject: [PATCH 7/7] Address AI reviewer feedback: fix configs, harden lint_docs, add Python 3.10+ guidance Co-Authored-By: Claude Opus 4.6 --- .coderabbit.yaml | 5 +++-- .github/CONTRIBUTING.md | 2 +- .github/copilot-instructions.md | 1 + .pr_agent.toml | 2 ++ toolchain/bootstrap/precheck.sh | 1 + toolchain/mfc/lint_docs.py | 4 +--- 6 files changed, 9 insertions(+), 6 deletions(-) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 5ab066e8c3..9921f27dfc 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -1,5 +1,5 @@ # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json -language: en-us +language: en-US reviews: profile: chill @@ -15,7 +15,8 @@ reviews: docs/documentation/contributing.md. - path: "toolchain/**/*.py" instructions: | - Python toolchain code. Follow PEP 8. + Python toolchain code. Follow PEP 8. Requires Python 3.10+; + do not suggest __future__ imports or backwards-compatibility shims. knowledge_base: code_guidelines: diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 05147a4144..e8907be817 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -2,7 +2,7 @@ We welcome contributions of all kinds -- bug fixes, new features, documentation, tests, and issue triage. -**Full developer guide:** [mflowcode.github.io/documentation/md_contributing.html](https://mflowcode.github.io/documentation/contributing.html) +**Full developer guide:** [mflowcode.github.io/documentation/contributing.html](https://mflowcode.github.io/documentation/contributing.html) ## Quick Reference diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 6ba315ca62..2dbfbbf331 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -19,6 +19,7 @@ Formatting and linting are enforced by pre-commit hooks. Focus review effort on * Only `simulation` (plus its `common` dependencies) is GPU-accelerated via **OpenACC**. * Code must compile with **GNU gfortran**, **NVIDIA nvfortran**, **Cray ftn**, and **Intel ifx**. * Precision modes: double (default), single, and mixed (`wp` = working precision, `stp` = storage precision). +* **Python toolchain** requires **Python 3.10+** — do not suggest `from __future__` imports or other backwards-compatibility shims. --- diff --git a/.pr_agent.toml b/.pr_agent.toml index 594b6629ae..1839914fcc 100644 --- a/.pr_agent.toml +++ b/.pr_agent.toml @@ -19,6 +19,8 @@ correctness (pack/unpack offsets, GPU data coherence), precision mixing consistency (pressure formula must match model_eqns), missing case_validator.py constraints for new parameters, and compiler portability across all four supported compilers. +Python toolchain requires Python 3.10+; do not suggest __future__ imports +or other backwards-compatibility shims. """ [pr_code_suggestions] diff --git a/toolchain/bootstrap/precheck.sh b/toolchain/bootstrap/precheck.sh index 41f0159294..9274f3abae 100755 --- a/toolchain/bootstrap/precheck.sh +++ b/toolchain/bootstrap/precheck.sh @@ -132,6 +132,7 @@ log "[$CYAN 5/5$COLOR_RESET] Checking$MAGENTA doc references$COLOR_RESET..." if python3 toolchain/mfc/lint_docs.py 2>&1; then ok "Doc references are valid." else + error "Doc reference check failed. Run$MAGENTA python3 toolchain/mfc/lint_docs.py$COLOR_RESET for details." FAILED=1 fi diff --git a/toolchain/mfc/lint_docs.py b/toolchain/mfc/lint_docs.py index 565b3f2332..4b46950905 100644 --- a/toolchain/mfc/lint_docs.py +++ b/toolchain/mfc/lint_docs.py @@ -11,8 +11,6 @@ "docs/documentation/running.md", "docs/documentation/case.md", ".github/copilot-instructions.md", - ".pr_agent.toml", - ".coderabbit.yaml", ] # Match backtick-wrapped strings that look like repo-relative file paths @@ -28,7 +26,7 @@ def check_docs(repo_root: Path) -> list[str]: doc_path = repo_root / doc if not doc_path.exists(): continue - text = doc_path.read_text() + text = doc_path.read_text(encoding="utf-8") for match in PATH_RE.finditer(text): path_str = match.group(1) if SKIP_RE.search(path_str):