Add CLAUDE.md guidance for agentic development with Claude Code#1690
Closed
glwagner wants to merge 65 commits into
Closed
Add CLAUDE.md guidance for agentic development with Claude Code#1690glwagner wants to merge 65 commits into
glwagner wants to merge 65 commits into
Conversation
Fix the 3-equation iteration for the buoyancy flux between the ocean and an overlying ice-shelf when ICE_SHELF_BUOYANCY_FLUX_ITT_BUGFIX is true and SHELF_3EQ_GAMMA it false. This code now uses proper bounding of the self-consistent solution, avoiding further amplifying the fluxes in the cases when the differences between the diffusivities of heat and salt to make the buoyancy flux destabilizing for finite turbulent mixing. Both the false-position iterations and the (appropriately chosen) Newton's method iterations have been extensively examined and determined to be working correctly via print statements that have subsequently been removed for efficiency. Previously, the code to determine the 3-equation solution for the buoyancy flux between the ocean and an ice shelf had been skipping iteration altogether or doing un-bounded Newton's method iterations with a sign error in part of the derivative, including taking the square root of negative numbers, leading to the issue described at NOAA-GFDL#945. That issue has now been corrected and can be closed once this commit has been merged into the dev/gfdl branch of MOM6. This commit also changes the names of the runtime parameters to correct the ice shelf flux iteration bugs from ICE_SHELF_BUOYANCY_FLUX_ITT_BUG and ICE_SHELF_SALT_FLUX_ITT_BUG to ICE_SHELF_BUOYANCY_FLUX_ITT_BUGFIX and ICE_SHELF_SALT_FLUX_ITT_BUGFIX to avoid confusion with other ..._BUG parameters where `true` is to turn the bugs on, whereas here `true` fixes them. The old names are retained via `old_name` arguments to the `get_param()` calls, so no existing configurations will be disrupted by these changes. Additionally, an expression to determine a scaling factor to limit ice-shelf bottom slopes in `calc_shelf_driving_stress()` was refactored to avoid the possibility of division by zero. This commit will change (and correct) answers for cases with ICE_SHELF_BUOYANCY_FLUX_ITT_BUGFIX set to true, but as these would often fail with a NaN from taking the square root of a negative value, it is very unlikely that any such configurations are actively being used, and there seems little point in retaining the previous answers. No answers are changed in cases that do not use an active ice shelf. Co-authored-by: Alistair Adcroft <adcroft@users.noreply.github.com>
- rotated OBC%segment%num_fields needs to be set.
Fix a bug that the recently changed default answer date for TIDES_ANSWER_DATE is not properly applied to MOM_self_attr_load. TIDES_ANSWER_DATE is used in MOM_self_attr_load to check if SAL_USE_BPA is used after a timestamp, so its default should be consistent with MOM_PressureForce_FV.
Thanks to both @alperaltuntas and @marshallward who noted that a PPM limiter has the expression `( u2 - u1 ) * ( u1 - u0 ) <- 0.0` which is interpreted as `( u2 - u1 ) * ( u1 - u0 ) < -0.0a. Needless to say, the intended code was `( u2 - u1 ) * ( u1 - u0 ) <= 0.0`. The same typo was copied to three files. The high-order estimate of edge value was previously bounded by (u2,u1) or (u1,u0). The missed conditions of either `( u2 - u1) == 0.` or `( u1 - u0 ) == 0.` would then have been caught by the subsequence test for an interior extrema. Thus, I think the cell was still limited to PCM appropriately. However, the typo obscured the intention of the limiter and I was lucky it still worked.
This commit allows the frequency-dependent drag to be implemented in tensor form, by incorporating the off-diagonal components of the wave drag tensor into the MOM_wave_drag module.
Recon1d_PLM_WLS provides a piecewise linear reconstruction where the slope is the "best" fit as determined by volume-weighted least squares. The reconstruction is NOT limited by neighboring cells. Therefore, this reconstruction is NOT useful for vertical remapping or grid generation. It is instead intended for the pressure gradient calculation; the idea is to disconnect the PLM slope from the values in vanish(ing) layers which appear to be the source of pressure-gradient errors over topographic slopes in z*-coordinate tests. Because the normal limiters do not apply, the only test I could think of was to check that the least squares fit was actually correct. The documentation explains how this was checked (which took a while due to round-off challenges with the loss function).
Corrected the descriptions of variable units in 64 comments spread across 16 files, including a dozen instances where "arbitrary" was misspelled. All answers are bitwise identical and only comments were changed.
Updated the values of about 21 parameters (many of which are repeated across TC test cases) used in the TC testing to test the most recent versions of code that is selected with ANSWER_DATE flags and to avoid testing the buggy versions of code that is regulated by _BUG flags. This includes some changes to broaden the range of equations of state that are being tested and to test some newer versions. This does change the details of the TC tests, but they should (and do) still pass TC regression tests across code versions.
* Added frazil to ice shelf The frazil mass flux to the ice-shelf base is calculated by multiplying frazil energy [J m-2] by the inverse of the timestep times the latent heat of fusion [kg J-1 s-1]. This frazil mass flux is incorporated as a negative water flux from the ice shelf. This negative water flux then acts to add the frazil mass to the ice shelf base (MOM_ice_shelf.F90/change_thickness_using_melt) and remove it from the ocean surface as evaporation (MOM_ice_shelf.F90/add_shelf_flux). Note frazil is reset to zero at the start of each therm timestep in MOM.F90/step_MOM. Some additional changes were also made to how the ice-shelf flux factor is implemented, so that is only scales ice-shelf melt without affecting the frazil mass flux. * Fixed a commented line where fluxes%water_flux should be ISS%water_flux
The PLM reconstruction used within the pressure gradient force now supports the weighted least squares approach for slope estimation. In a catastrophic version of seamount/z where vanished layers slightly inflate, the regular finite volume PLM method is sensitive to values in the vanished layers and leads to a feedback that causes en error growth (spontaneous motion). The PLM-WLS method is insensitive to the vanished layers and in the same test leads only to round-off level noise in the flow.
* Spatially varying bottom drag coefficient The spatially varying bottom drag coefficient can be specified by providing a map of the spatially varying scaling factor. * Spatially varying bottom drag coefficient Fixed the inconsistency at open boundaries when CDRAG_MAP is true.
In a number of cases, total resting column thickness is calucated as
G%bathyT + G%Z_ref, which is largely correct but for wetting, i.e.
G%bathyT < 0. This commit makes a correction for seven cases with this
potential bug.
There is no answer changes if no wetting points are used and G%Z_ref is
zero.
List of modules/processes affected:
* MOM_barotropic
* affects only surface stress when BT_NONLIN_STRESS is False.
* MOM_wave_speed
* h2 calculations in
* subroutine internal_tides_init
* subroutine int_tide_input_int
* subroutine tidal_mixing_init
* MOM_lateral_mixing_coeffs
* MOM_MEKE
In commit b8c807b, we made the test for SSH penetrating the sea floor when using BT_LIMIT_INTEGRAL_TRANSPORT because we thought it could never happen. Unfortunately, floating-point round off allows violations and we were hitting the now fatal error. This commit calculates the precision we can expect for the current SSH and then if the ocean thickness has become negative within this precision, we reset to zero thickness. This should not change answers in that BT_LIMIT_INTEGRAL_TRANSPORT is a new option, and if anyone was using it they would have encountered a FATAL, and this fix does not alter any positive thicknesses.
When debugging with all run-time tests turned on, the integer `num_lines` was flagged as used but uninitialized when being passed to `broadcast()`. I don't think the code was wrong, just that the checks expected the "inout" argument to be set on all processors when the purpose of `broadcast()` is to take the value from root_PE and send to everyone else. I don't know why this hadn't been detected before - maybe compiler version. The fix is trivial and has no impact on production codes or answers.
`oldfn` was not initialized when used in a logical test. This did not matter for numerical results; the logical expression always evaluated to the False correctly due to other parts of the expression. Nevertheless, this variable was technically used uninitialized and a debugging executable doesn't get past this. Hence the fix.
When debugging the ice sheet configuration, a non-zero barotropic transport could not be reconciled with the layer transports because the derivative of net layer transports was zero (d/dv hu). This arose due to all layer flows pointed from vanished to thick so that their marginal thicknesses were individually zero. Adding a floor to the marginal thickness allows the solver to find the adjustment that does reconciles the two estimates. I've made this optional via parameter CONT_USE_H_MARG_MIN, and with default of False. If this situation had occurred before, we surely would have had a crash so it's likely that always applying this floor would not change answers. However, there's the weak possibility that a teeny-tiny transport, smaller than H_subroundoff, has existed in a run and then this answer would change. With the default of False we can be sure there are no answer changes, but it is recommended to use this option for safety.
The CHANNEL_DRAG option was using a harmonic mean to interpolate adjacent bottom depths at velocity points to vorticity points. However, this is not well behaved when the bottom depth is negative (i.e., above sea level), as was noted as a part of PR mom-ocean#975. This commit adds the new runtime parameter CHANNEL_DRAG_SHELFBREAK_DEPTH to set a depth below which a harmonic mean bottom depth is still used to mimic a continental shelfbreak profile, but above which a simple arithmetic mean is used to interpolate bathymetry to vorticity points for use with CHANNEL_DRAG. The expressions vary continuously with depth and avoid the previous problems with division by zero or a badly formed harmonic mean. By default, all answers are bitwise identical in any cases that worked previously, but cases with oceans (or Great Lakes) in basins with bottoms that are above sea-level should now work sensibly when CHANNEL_DRAG is enabled. There is a new runtime parameter in some cases.
Corrected the incorrect or inconsistent unit descriptions of 28 variables, added descriptions of the units of 4 others, and corrected the non-standard syntax (e.g. backwards or in the wrong order) in the description of 35 variables, scattered across 27 files. Only comments are changed and all answers are bitwise identical.
* Fix for ice-shelf friction velocity bugs Fixed an incorrect area used to calculate cell-centered ocean surface velocity under the ice_shelf, which can impact the calculation of ice-shelf friction velocity. Added missing flags to some allocate_surface_state calls so that sfc_state%taux_shelf and sfc_state%tauy_shelf are allocated. This is required for the surface-stress-based (rather than surface-velocity-based) calculation of ice-shelf friction velocity. Also added taux_shelf and tauy_shelf as diagnostics for the surface stress under the ice shelf. * Removed unneeded taux_shelf and tauy_shelf diagnostics * Added ustar_from_vel_bugfix flag, which if true, fixes the ustar from ocean velocity bug
This patch undoes a coupling of the FMS infra layer to the MOM6 framework code. In the current FMS infra layers, the `get_extern_field_info()` and `init_extern_field()` functions require content defined in `src/framework`. This prevents the development of new independent infra layers, which much also depend on infra-agnostic content. In particular, the FMS2 implementation of `get_extern_field_axes()` relies exclusively on the framework function, `get_var_axes_info()`. Both infras also return the `axes_info` type, a MOM-specific framework-level descriptor, rather than the infra `axistype`. This patch resolves these inconsistencies. * `axis_info` no longer appears at infra-level. All relevant functions now reference `axistype`. * `src/framework/MOM_io.F90` now provide functions for translating `axistype` to `axis_info`. Some specific changes are summarized below. * `get_external_field_info` is now a framework-level function of `MOM_interpolate.F90` , using infra-level implementations of `get_extern_field_(size|axes|missing)`. Each is now explicitly defined at the infra-level. * The FMS2 `get_external_field_axes` is now an entirely new function, and is largely a duplicate of `get_var_axes_info()`. The major difference is that it returns a list of `axistype`. It also replaces the fixed x-y-z fetch with a slightly more generic list of axes. (It still requires at least three dimensions, however.) * `set_axis_data` is only used internally by the FMS2 infra. It is included in FMS1 but raises an nonimplementation error. There is one minor API change. * The `name` argument was added to `get_axis_data`. It is now the second argument, to match the style of existing functions, and size was moved to the third argument. Other minor framework references have been removed. * `MOM_error` and `FATAL` now refernce their `MOM_error_infra` equivalents. * `lowercase`, which was previously only defined in FMS1, has been added to the FMS2 infra. Note that this is a duplication of the function in `src/framework/MOM_string_functions.F90`.
When a z-coordinate diagnostic grid is specified via the "PARAM" method of coordinate definition, then the number of levels was always the same as the main model. This commit fixes this by first allowing for upto a 1000 levels in the new grid, checking for the actual requested size, and then allocating to that size. It appears we have no examples using this mode, which is probably how this bug has persisted so long. This "PARAM" method of specifying grids is being used in a range of new CMIP7 diagnostics in both MOM6 and COBALT.
…mom-ocean#1003) * Init all sponge tendency diag IDs to -1 immediately * No need to reset to -1 since initialized when declared * Move init_ALE_sponge_diags to after all tracers have been set up
These two references to members of a pointer don't seem to be hit except under special circumstances but nevertheless I ran in to them when debugging an unrelated problem. There are two references to members of `diag%axes` that assume `diag%axes` are associated, but in the specific case I was debugging this was not the case.
Five vertically integrated diagnostics are requested in CMIP7. These ultimately are to be for four vertical intervals (0-300m, 300-700m, etc.) but we will handle that through addition of a 4-level diagnostic grid, configured at run-time. This commit handles the conversion from temperature or salt to heat content or salt content (by mass) and registers a "vertically extensive" quantity so that the diagnostics know to re-integrate rather than remap. Changes: - Added diagnostics absscint, pfscint, scint, chcint and phcint - Moved registration of temp_int and salt_int to within an existing `if (use_temperature)` block - Made public 2 GSW conversion functions in MOM_EOS
…roducing_sum (and therefore, mpp_sum) is called. Previously, several 2-D arrays were each being passed within their own reproducing_sum calls, which is now avoided by consolidating the 2-D arrays into one 3-D array that is passed to a single reproducing_sum call.
…ux calculation. Needed for runs without frazil.
This commit uses G%meanSL in 13 modules. The change is essentially replacing G%bathyT + G%Z_ref with G%meanSL + G%bathyT. Note that this does NOT mean parameter G%Z_ref is replaced by G%meanSL. G%Z_ref is factored in both G%meanSL and G%bathyT and it is kept as a useful consistency testing tool. Another cosmetic change is made by using G%meanSL + G%bathyT, instead of G%bathyT + G%meanSL, which (hopefully) can be easily interpreted as G%meanSL - (-G%bathyT).
max_depth is really used as a maximum static thickness throughout the model, so meanSL needs to be considered.
At no point does MOM6 code actually set arrays passed to the post_data() to have a missing value. Instead a missing value is set in output files entirely by masking. This commit eliminates the logic that would (inaccurately) try to reset fields that seem to match rescaled missing values to the output missing value. The previous code was inaccurate, in that a rescaled field could have taken on the unscaled missing value as a valid data point and still have been incorrectly marked is missing, although the odds of this happening are exceptionally small and it would only be cases with dimensional rescaling where this could have applied. For 2-d diagnostics, this commit eliminates a duplicative array syntax math expression that did exactly what the code now does. All solutions are identical, and because the missing value was not being explicitly it is unlikely that any diagnostics will change.
…se it is already accounted for in fluxes%sens
Added the new elements `IdxCu_OBCmask` and `IdyCv_OBCmask` to the `ocean_grid_type` and `dyn_horgrid_type` to facilitate the application of no-gradient open boundary conditions at faces with essentially no added overhead. These new arrays are set initially in `set_derived_metrics()` and `set_derived_dyn_horgrid()`, but may be reset in `initialize_masks()` and `open_boundary_impose_land_mask()`. All answers are bitwise identical but there are a pair of new 2-d arrays in two transparent grid types.
Modified the code to use `G%IdxCu_OBCmask` and `G%IdyCv_OBCmask` in 7 places each in 6 modules. They are used instead of `G%OBCmaskCu*G%IdxCu` and `G%OBCmaskCv*G%IdyCv`, to which they are equivalent. This change should slightly speed up the model, and as expected all answers are bitwise identical.
* Add option to scale tidal amplitude for bottom ustar. - previously we used the tidal amplitude to compute ustar. - The additional factor translates between amplitude and time mean tidal current. - Setting the factor TIDEAMP_FACTOR<0 preserves old answers. * Update tideamp factor implementation for efficiency - factor out the negative "default" value to automatically set to multiply by 1.0 instead of using an if-block. - factor in the c-grid averaging 0.5 to further reduce extra operations, but clearly label the parameter to reflect this. --------- Co-authored-by: brandon.reichl <brandon.reichl@noaa.gov>
* Add vertical tracer flux diagnostic for dye tracers - Register vertical flux diagnostic in initialize_dye_tracer - Calculate net vertical flux from entrainment (positive upward) - Post flux diagnostic in dye_tracer_column_physics * changed diagnostic registration to be at interface, made sure boundary fluxes are zero * changed lines 338 and 354 as needed. Fixed accidental space on Line 1.
This commit is meant to fix the issue that all parameters in MOM_initialize_fixed after OBC are logged under module MOM_open_boundary in MOM_parameter_doc. By moving log_version call after OBC, parameters from MOM_initialize_fixed are now logged under three "modules" in MOM_parameter_doc: 1. Parameters before OBC are under module MOM_grid_init, which also (incorrectly) includes topography relatd parameters. 2. module MOM_open_boundary 3. Parameters after OBC are under module MOM_initialize_fixed. The change makes sure OBC parameters are well separated from the other parameters. This is a hack rather than a fix.
* Make OBC related calls in MOM_initialize_fixed explicitly conditional for readibility. * Early return in open_boundary_config if there is no segment, so that OBC is not allocated and then deallocated.
Refactored gradKE() to only take a single layer of velocities and thicknesses and to use G%IdxCu_OBCmask and G%IdyCv_OBCmask to avoid extra loops over the OBC segments for efficiency. With these changes, there is also an expectation that gradKE() will be more likely to be inlined or threaded and hence become more efficient. All solutions are bitwise identical, but there are some diagnostics of accelerations that are fully or partially set to zero at solid (masked) boundaries.
This patch significantly modifies the Autoconf-based builds to separate external content in `pkg/` from the main MOM6 source code. These changes are meant to address the challenges with analysis of source code which is outside of our control. Such content is now built separately, isolating it from the usual MOM6 code requirements. A secondary benefit is that a submodule checkout is no longer required for ocean-only builds. The patch includes the following specific changes. * Builds relying on `ac/deps/` now build libraries for GibbsSeaWater (`libgsw.a`) and CVMix (`libcvmix.a`) alongside `libFMS.a`. This replaces any content placed externally into `pkg/`. * A new macro, `MOM6_FC_CHECK_LIB`, improves library detection tests by including support for both subroutines and functions, as well as generic argument lists. * Makefiles across the build system (`ac/deps/`, `.testing/`, etc.) have been updated to generate and use the new libraries. These changes are not intended to break existing builds, as summarized below. * `src/parameters/CVMix/` and `TEOS10/` are explicitly excluded from the makedep source trees, but the symbolic links to `pkg/` are retained, and existing builds which assume their presence should still work. * The legacy `AX_FC_CHECK_LIB` macro has been retained, so that it remains available to any external MOM6-examples builds. * The git submodules have not been removed, although the GitHub Actions no longer uses then in testing. The patch also includes the following related minor modifications. * The GitHub Actions FMS build stages now includes GSW and CVMix * `.gitignore` files are modified to target files produced by the build system, rather than globbing of various incidental files. * A non-POSIX shell operation `==` in `configure.ac` has been replaced.
`ac/deps/Makefile` and `.testing/Makefile` now provide optional configuration of a target `pkg/` directory, using the PKG macro. To build using the submodule source code, PKG is unset:: make -j In this case, symbolic links to `pkg/` are used to build CVMix and GSW libraries. Modifications to the equivalent files in `src/` will be seen by the library builds. Developers who prefer to edit symbolic links from rogue submodules can still work as before. To build using `libcvmix.a` and `libgsw.a`, unset `PKG`:: PKG= make -j or make -j PKG= To accommodate this, there are now explicit macros `MAKEDEP` to specify the path of `makedep`. This prevents some of the "pathing up" issues that should never have been used, and were exposed by the new symlinks in to `pkg/`. In just about every normal use case, these are pre-configured. Macros for Python and autoconf detection were also modified to check preset `PYTHON` and `MAKEDEP` settings before attempting to search `PATH`. Following autoconf convention, the preset values of `PYTHON` and `MAKEDEP` are not reported.
Eliminated about 170 unused internal variables scattered across 26 files. Most of these were deleted outright, but a few that are associated with commented out blocks of code were just commented out. All answers are bitwise identical.
This is a follow up to commit 27f4243 (break down for easy review). if (OBC%number_of_segments > 0) is removed from open_boundary_config, so that the indentation level can be reduced for most part of the routine.
It should be in thickness unit (m for Boussinesq and kg/m2 for non-Boussinesq). The long name description is also slightly changed.
The two pairs are used for different purposes. `[uv]bt_st`` are used for diagnosing barotropic momentum tendency and `[uv]bt_first` are used for OBC. But they are equivalent (initial BT velocity) and neither uses wide halo. This commit merges the two. Extra halo points in assigning `[uv]bt_st` are also removed.
Add a comprehensive guide for AI-assisted MOM6 development, covering coding conventions, unit documentation system, arithmetic reproducibility rules, module structure patterns, diagnostics, testing infrastructure, and contribution workflow. Derived from analysis of lead developer PRs (Hallberg, Adcroft), the MOM6 wiki, ReadTheDocs, and codebase patterns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Added sections on: infrastructure layering (infra must never import from framework), defensive programming (allocated checks, type-correct comparisons, error message conventions), diagnostics masking (never set missing values, automatic mask handling), answer-changing parameter system (_BUG flags and ANSWER_DATE conventions), and expanded common pitfalls with soft case enforcement and public scope minimization. Patterns drawn from PRs by marshallward, herrwang0, JorgeG94, c2xu, and additional Hallberg/Adcroft PRs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
I'll reopen this pointing to dev/gfdl (where I started from) but happy to take advice where this should point (it just adds one file so it should be trivial...) |
Contributor
|
I would much rather have skills associated with common MOM6 activities and put them in a separate repository. CLAUDE.md is not the right place for this. CLAUDE.md should be reserved for individual preferences and instructions on how they wish to interact with the code. CLAUDE.md is also always read on startup by Claude and will cost users tokens and pollute their context window. I vote in favor of preserving flexibility for users. Skills are a far better, more controlled way to go. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
12 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds a
CLAUDE.mddocument to help guide development with Claude Code. This document is loaded into the context of the model Claude is using when its started up inside the MOM6 directory. An introduction to the basic purpose ofCLAUDE.mdis written here:https://www.humanlayer.dev/blog/writing-a-good-claude-md
The reason to add this to the repo itself is because, in addition to providing MOM6-specific guidance to Claude for any development task, the
CLAUDE.mdcan also be used to catch "common mistakes" that Claude makes (eg it can say "make sure you don't do this"). This is actually not foolproof; sometimes Claude will make a mistake even if it is explicitly told not to. However, this can often be fixed with something like "review the code you've written and make sure you don't make any of the mistakes in CLAUDE.md". The common mistakes are things that have to be found empirically by using CLAUDE.md, so including it in the repo allows all MOM6 develops to collaborate on strengthening and improving the Claude dev workflow by contributing toCLAUDE.md.Including this was an idea that came out of a conversation with @StephenGriffies !
I'll note that one can go much further with guidelines for Claude; for example many .md files can be developed for specific tasks. And so on.