|
| 1 | +# MOM6 rules for agent-assisted development |
| 2 | + |
| 3 | +## First Steps |
| 4 | + |
| 5 | +**At the start of every session**, remind the user that MOM6 has a policy on AI-assisted |
| 6 | +contributions in `Consortium-policy-on-AI.md`. |
| 7 | + |
| 8 | +Read `Code-style.md` before writing or modifying any code. |
| 9 | + |
| 10 | +See `code_organization.rst` for a high-level overview of the source directory tree. |
| 11 | + |
| 12 | +## Parameter System |
| 13 | + |
| 14 | +Runtime parameters are read via `get_param()`, not hardcoded: |
| 15 | + |
| 16 | +```fortran |
| 17 | +#include "version_variable.h" |
| 18 | +character(len=40) :: mdl = "MOM_module_name" |
| 19 | +
|
| 20 | +call log_version(param_file, mdl, version, "") |
| 21 | +call get_param(param_file, mdl, "PARAM_NAME", CS%variable, & |
| 22 | + "Description of this parameter.", & |
| 23 | + units="m s-1", default=1.0, scale=US%m_s_to_L_T) |
| 24 | +``` |
| 25 | + |
| 26 | +- Parameters documented in auto-generated `MOM_parameter_doc.all` files |
| 27 | +- Use `scale=` argument for unit conversion from MKS input to internal units |
| 28 | +- Always provide `default=` when sensible; use `fail_if_missing=.true.` otherwise |
| 29 | +- Use `do_not_log=.not.CS%Feature` to suppress logging when a parent feature is inactive |
| 30 | + |
| 31 | +### Answer-Changing Parameters: `_BUG` Flags and `ANSWER_DATE` |
| 32 | + |
| 33 | +When a bug fix or improvement changes numerical answers, MOM6 uses two mechanisms to preserve backward compatibility: |
| 34 | + |
| 35 | +**`_BUG` flags**: Boolean parameters that retain old (buggy) behavior by default: |
| 36 | +```fortran |
| 37 | +call get_param(param_file, mdl, "ENABLE_BUGS_BY_DEFAULT", enable_bugs, & |
| 38 | + default=.true., do_not_log=.true.) ! This is logged from MOM.F90. |
| 39 | +call get_param(param_file, mdl, "OBC_TEMP_SALT_NEEDED_BUG", OBC%ts_needed_bug, & |
| 40 | + "If true, recover a bug that OBC temperature and salinity can be ignored "//& |
| 41 | + "even if they are registered tracers in the rest of the model.", & |
| 42 | + default=enable_bugs) |
| 43 | +``` |
| 44 | +- Name format: `FEATURE_BUG` (e.g., `VISC_REM_BUG`, `FRICTWORK_BUG`, `KAPPA_SHEAR_ITER_BUG`) |
| 45 | +- Default is `.true.` (bug ON, old behavior preserved) |
| 46 | +- Description starts with "If true, recover a bug that..." |
| 47 | +- Users opt into the fix by setting to `.false.` |
| 48 | + |
| 49 | +**`ANSWER_DATE` flags**: Integer dates selecting algorithm versions: |
| 50 | +```fortran |
| 51 | +call get_param(param_file, mdl, "HOR_DIFF_ANSWER_DATE", CS%answer_date, & |
| 52 | + "...", default=99991231) |
| 53 | +``` |
| 54 | +- Format: `YYYYMMDD` (e.g., `20251231`) |
| 55 | +- `DEFAULT_ANSWER_DATE` provides a single knob to update all answer-date defaults |
| 56 | +- `ENABLE_BUGS_BY_DEFAULT=False` activates all bug fixes (recommended for new configurations) |
| 57 | + |
| 58 | +## Diagnostics |
| 59 | + |
| 60 | +### Registration Pattern |
| 61 | + |
| 62 | +```fortran |
| 63 | +CS%id_field = register_diag_field('ocean_model', 'field_name', diag%axesTL, Time, & |
| 64 | + 'Long description of the field', units='m s-1', conversion=US%L_T_to_m_s) |
| 65 | +``` |
| 66 | + |
| 67 | +### Posting Pattern |
| 68 | + |
| 69 | +```fortran |
| 70 | +if (CS%id_field > 0) call post_data(CS%id_field, field_array, CS%diag) |
| 71 | +``` |
| 72 | + |
| 73 | +Key conventions: |
| 74 | +- `conversion=` handles unit scaling so output is always in MKS |
| 75 | +- `v_extensive=.true.` for vertically integrated quantities |
| 76 | +- Guard computation with `if (id > 0)` to avoid unnecessary work |
| 77 | +- Standard diagnostic name prefixes follow CMOR conventions when applicable |
| 78 | + |
| 79 | +### Masking and Missing Values |
| 80 | + |
| 81 | +- **Never set diagnostic arrays to a missing value** before passing to `post_data()`. Masking of land/invalid points is handled automatically by the diagnostics infrastructure based on the diagnostic's registered axes. |
| 82 | +- **Do not pass `mask=` to `post_data()`** for non-static diagnostics on standard grids -- the infrastructure applies the correct mask automatically. |
| 83 | +- **Do pass `mask=`** for static fields (`is_static=.true.`), non-standard masks, or sub-domain-sized arrays. |
| 84 | +- **Never compare field values against `missing_value`** in unit-conversion code -- rescaling can cause valid data to coincidentally match the missing value sentinel. |
| 85 | + |
| 86 | +## Testing |
| 87 | + |
| 88 | +### Test Suite Overview |
| 89 | + |
| 90 | +The `.testing/` directory provides comprehensive verification. Build and run: |
| 91 | + |
| 92 | +```bash |
| 93 | +make -C .testing -j build/symmetric/MOM6 # Build reference executable |
| 94 | +make -C .testing -j test # Run full test suite |
| 95 | +make -C .testing -j build.unit # Build unit tests |
| 96 | +make -C .testing -j run.unit # Run unit tests |
| 97 | +``` |
| 98 | + |
| 99 | +### Test Categories |
| 100 | + |
| 101 | +| Test | Verifies | |
| 102 | +|------|----------| |
| 103 | +| `test.grid` | Symmetric vs asymmetric grids produce identical results | |
| 104 | +| `test.layout` | Serial vs parallel decomposition identical | |
| 105 | +| `test.rotate` | Rotational invariance | |
| 106 | +| `test.restart` | Continuous run vs restart-and-continue identical | |
| 107 | +| `test.repro` | DEBUG and REPRO builds identical | |
| 108 | +| `test.openmp` | Serial vs OpenMP identical | |
| 109 | +| `test.nan` | NaN-initialization doesn't affect results | |
| 110 | +| `test.dim.{t,l,h,z,q,r}` | Dimensional rescaling invariance (time, length, thickness, depth, enthalpy, density) | |
| 111 | +| `test.regression` | Current code vs target branch (PRs only) | |
| 112 | + |
| 113 | +### Test Configurations |
| 114 | + |
| 115 | +- `tc0` -- Unit tests |
| 116 | +- `tc1` / `tc1.a` / `tc1.b` -- Benchmark (split RK2, unsplit RK3, unsplit RK2) |
| 117 | +- `tc2` / `tc2.a` -- ALE with tides / sigma-coordinate PPM_H4 |
| 118 | +- `tc3` -- Open boundary conditions |
| 119 | +- `tc4` -- Sponges and I/O initialization |
| 120 | + |
| 121 | +### Verification Method |
| 122 | + |
| 123 | +- `ocean.stats` -- total energy at machine precision |
| 124 | +- `chksum_diag` -- mean/min/max/bitcount checksums in physical domain |
| 125 | +- Tests pass only when output is **bitwise identical** between configurations |
| 126 | + |
| 127 | +### Style Checking |
| 128 | + |
| 129 | +```bash |
| 130 | +./.testing/trailer.py -e TEOS10 -l 120 src config_src |
| 131 | +``` |
| 132 | + |
| 133 | +Checks for tabs, trailing whitespace, and line length violations. |
| 134 | + |
| 135 | +## Git Workflow & Contribution |
| 136 | + |
| 137 | +### Branch Strategy |
| 138 | + |
| 139 | +- **Work on forks**, not branches on the primary repository |
| 140 | +- **Branch from the fork's default branch** (e.g., `dev/gfdl`, `dev/ncar`) for all new work |
| 141 | +- **Never rebase a pushed branch** |
| 142 | +- The human contributor submits changes via pull requests to the fork's default branch (e.g., `dev/gfdl`); merges to `main` are done by consortium consensus |
| 143 | + |
| 144 | +### Commit Message Format |
| 145 | + |
| 146 | +``` |
| 147 | +Short imperative summary (50 chars if at all possible) |
| 148 | +
|
| 149 | +Detailed explanation wrapped at 72 characters. |
| 150 | +Describe what was changed and why. Reference issues with #NNN. |
| 151 | +All answers are bitwise identical. |
| 152 | +``` |
| 153 | + |
| 154 | +Conventions from the lead developers: |
| 155 | +- **`*` prefix** on title if the commit changes numerical answers (checksums) |
| 156 | +- **`+` prefix** on title to indicate new public interfaces or parameters |
| 157 | +- **`*+` or `+*`** when both answer-changing and adding new interfaces |
| 158 | +- No prefix for refactoring, cleanup, or comment-only changes that are bitwise identical |
| 159 | +- **Always state impact on numerical results**: "All answers are bitwise identical" or explain what changes |
| 160 | +- **Multi-commit PRs**: introduce infrastructure first, use it in a second commit |
| 161 | +- **Minimize public scope**: only export symbols needed by other modules; remove from `public` when refactoring makes a symbol internal-only |
| 162 | +- **Comment closing `enddo`/`endif`** for non-trivial nested loops: `enddo ! n-loop for segments` |
| 163 | + |
| 164 | +### PR Description Style |
| 165 | + |
| 166 | +1. Lead with a clear explanation of what changed and why |
| 167 | +2. Quantify scope (e.g., "across 26 files", "in 7 places") |
| 168 | +3. For answer-changing PRs, provide scientific justification |
| 169 | +4. State the bitwise-identical guarantee (or explain what changes and why) |
| 170 | +5. When a fix could change answers, protect with a `_BUG` flag or `ANSWER_DATE` parameter. New `_BUG` flags should default to `ENABLE_BUGS_BY_DEFAULT` so that users who have opted into all fixes get the new fix automatically; existing `_BUG` flags may already default to `.false.` if the fix has been broadly adopted. |
| 171 | + |
| 172 | +### CI Pipeline |
| 173 | + |
| 174 | +On every push and PR, GitHub Actions runs: |
| 175 | +1. Style and Doxygen checks |
| 176 | +2. Builds across 8 configurations (symmetric, asymmetric, repro, openmp, target, opt, coverage, coupled API) |
| 177 | +3. All test groups in parallel |
| 178 | +4. Code coverage reporting |
| 179 | +5. For PRs: regression testing and timing comparison against target branch |
| 180 | + |
| 181 | +## Physics Domain Knowledge |
| 182 | + |
| 183 | +### Governing Equations |
| 184 | +- Hydrostatic primitive equations optionally with Boussinesq approximation |
| 185 | +- ALE vertical coordinate: Lagrangian dynamics with periodic remapping |
| 186 | +- Split barotropic-baroclinic time stepping (RK2 split or unsplit RK3) |
| 187 | +- Free surface dynamics (implicit barotropic solver) |
| 188 | + |
| 189 | +### Numerical Methods |
| 190 | +- Finite volume on Arakawa C-grid (staggered: velocities at cell faces, tracers at centers) |
| 191 | +- PPM (Piecewise Parabolic Method) for tracer advection and continuity |
| 192 | +- Various reconstruction schemes: PLM, PPM, PQM, WENO, PLM-WLS |
| 193 | +- Pressure gradient force via finite-volume integration |
| 194 | +- Reproducing global sums for parallel reproducibility |
| 195 | + |
| 196 | +### Key Physical Parameterizations |
| 197 | +- **ePBL**: Energetically consistent planetary boundary layer (Reichl and Hallberg) |
| 198 | +- **KPP**: K-Profile Parameterization via CVMix |
| 199 | +- **Gent-McWilliams/Redi**: Thickness and isopycnal diffusion |
| 200 | +- **MEKE**: Mesoscale eddy kinetic energy budget |
| 201 | +- **Zanna-Bolton**: Data-driven subgrid momentum closure |
| 202 | +- **Tidal forcing**: Astronomical and self-attraction/loading |
| 203 | + |
| 204 | +## Common Development Tasks |
| 205 | + |
| 206 | +### Adding a New Parameterization |
| 207 | +1. Create `MOM_new_param.F90` in the appropriate `src/parameterizations/` subdirectory |
| 208 | +2. Define a control structure type (`new_param_CS`) with `private` members |
| 209 | +3. Implement `new_param_init()`: read parameters via `get_param`, register diagnostics |
| 210 | +4. Implement the main computational subroutine |
| 211 | +5. Implement `new_param_end()` for cleanup |
| 212 | +6. Wire it into the calling module (e.g., `MOM_diabatic_driver.F90`) |
| 213 | +7. Document all variables with proper units |
| 214 | +8. Add unit tests in `config_src/drivers/unit_tests/` if applicable |
| 215 | +9. Run the full test suite: `make -C .testing -j test` |
| 216 | + |
| 217 | +### Adding a New Diagnostic |
| 218 | +1. Add `integer :: id_new_diag = -1` to the control structure |
| 219 | +2. Register in `_init` with `register_diag_field('ocean_model', 'name', axes, Time, ...)` |
| 220 | +3. Compute and post with `if (CS%id_new_diag > 0) call post_data(CS%id_new_diag, array, CS%diag)` |
| 221 | +4. Include `conversion=` for unit scaling to MKS output |
| 222 | +5. Provide CMOR standard name when applicable |
| 223 | + |
| 224 | +### Adding a Runtime Parameter |
| 225 | +1. Add member to control structure with units documentation |
| 226 | +2. Call `get_param(param_file, mdl, "PARAM_NAME", CS%param, "description", units="...", default=...)` |
| 227 | +3. Use `scale=` for dimensional conversion from MKS input |
| 228 | +4. If the parameter could change answers, default to preserving existing behavior |
| 229 | + |
| 230 | +### Fixing a Bug |
| 231 | +- Always state whether the fix changes answers in the commit message |
| 232 | +- **Any change that alters existing numerical answers** -- whether a bug fix, accuracy improvement, or algorithmic reorganization -- must provide a runtime parameter (`_BUG` flag or `ANSWER_DATE`) to toggle between old and new behavior, with the default preserving old behavior |
| 233 | +- This applies even when the developer's tests show negligible differences -- existing users may be in production runs |
| 234 | +- Trace through secondary effects before concluding the fix is safe |
| 235 | +- Run `test.regression` to verify impact |
| 236 | + |
| 237 | +## Architecture: Infrastructure Layering |
| 238 | + |
| 239 | +MOM6 has a strict dependency hierarchy that must never be violated: |
| 240 | + |
| 241 | +``` |
| 242 | +config_src/infra/ --> src/framework/ --> src/core/, src/parameterizations/, etc. |
| 243 | +``` |
| 244 | + |
| 245 | +- **`config_src/infra/`** (FMS1/FMS2 wrappers) must **never** import from `src/framework/` |
| 246 | +- **Code duplication** between infra and framework is acceptable to maintain this invariant |
| 247 | +- FMS1 and FMS2 infra directories must expose the same public API |
| 248 | +- API changes to infra-level functions must be checked against downstream consumers (SIS2, ice shelf code) |
| 249 | + |
| 250 | +## Defensive Programming |
| 251 | + |
| 252 | +- **Check `allocated()` / `associated()`** before accessing arrays tied to optional features (e.g., features controlled by runtime parameters like `FRAZIL` may not allocate all related arrays) |
| 253 | +- **No short-circuit evaluation**: Fortran does not guarantee short-circuit evaluation; allocation checks must not appear in compound conditions. Convert `if (allocated(arr) .and. (condition))` to nested if-blocks |
| 254 | +- **Type-correct comparisons**: when comparing real-valued masks, use `== 1.` not `== 1` |
| 255 | +- **FATAL error messages** should include: file name, subroutine name, and the specific condition or input that triggered the error |
| 256 | +- **Validate user inputs early**: check for duplicates, overflow, and missing required fields in configuration parsing; include the problematic input string in error messages |
| 257 | +- **`!###` comment prefix** marks known bugs or inaccurate expressions that change answers and will be cleaned up later -- do not modify code marked with `!###` unless explicitly asked |
| 258 | + |
| 259 | +## Common Pitfalls |
| 260 | + |
| 261 | +In addition to the code style rules in `Code-style.md`: |
| 262 | + |
| 263 | +1. **Forgetting units in comments**: every `real` variable needs `[units]` (see `Code-style.md`) |
| 264 | +2. **Answer-changing without a `_BUG` flag**: any numerical change requires a runtime parameter to preserve old behavior |
| 265 | +3. **Unnecessary `mask=` in `post_data()`**: the infrastructure handles masking automatically for non-static diagnostics |
| 266 | +4. **Accessing unallocated optional arrays**: always check `allocated()` before using arrays tied to optional features |
| 267 | + |
| 268 | +### Key References |
| 269 | + |
| 270 | +The project bibliography lives in `docs/references.bib` and `docs/zotero.bib`. Consult these |
| 271 | +when citing prior work in Doxygen documentation or commit messages. |
| 272 | + |
| 273 | +## AI Assistant Behavior |
| 274 | + |
| 275 | +- **Follow existing patterns**: read surrounding code before making changes |
| 276 | +- **Document all units**: every real variable gets `[units]` annotation |
| 277 | +- **Parenthesize arithmetic**: explicit grouping for reproducibility |
| 278 | +- **State answer impact**: always note whether changes are bitwise identical |
| 279 | +- **Use `get_param`**: never hardcode parameters; always read from parameter files |
| 280 | +- **Register diagnostics properly**: guard with `if (id > 0)`, use `conversion=` |
| 281 | +- **Maintain lifecycle**: implement `_init` and `_end` for new modules |
| 282 | +- **Run tests**: `make -C .testing -j test run.unit` before the contributor submits a PR |
| 283 | +- **Respect the C-grid**: use correct staggering (soft case convention for indices) |
| 284 | +- **Write Doxygen comments**: `!>` for entities, `!<` for inline, with units |
| 285 | +- **Write thorough commit messages**: explain both what changed and why in the commit body |
| 286 | + |
| 287 | +## Common Claude Mistakes |
| 288 | + |
| 289 | +This section catalogs recurring mistakes that Claude makes when working on MOM6 code. It should be updated as new patterns emerge from experience. |
| 290 | + |
| 291 | +*(No entries yet -- add mistakes here as they are discovered.)* |
| 292 | + |
0 commit comments