Conversation
Test write_avg_type/read_avg_type verify that the timestamp handling bug fix in setup_file works correctly: for AVERAGE/MIN/MAX output with monthly storage, each month's data must go into a separate file (exactly 1 snapshot per file), named using the start of the averaging window (last_write_ts). Before the fix, setup_file used next_write_ts (end of window) to set time_idx, causing snapshot_fits to keep the file open for an extra month, resulting in multiple snapshots accumulating in a single file.
Deletes files from gcam/tools that now exist in a different repo - https://github.com/E3SM-Project/ehc-misc-scripts
…-Project#8592) Update the logic for setting the time index in the setup_file function of eamxx_output_manager.cpp to ensure correct handling of time indices for different output averaging types. Output time index handling: - Updated filespecs.storage.set_time_idx to use control.next_write_ts for Instant output, and control.last_write_ts for average/min/max outputs, ensuring the correct timestamp is used for each averaging type.
Previously, every MPI rank independently opened the same homme_log_fname file (root with status='REPLACE', all others with status='OLD'), each getting its own file description/offset. Since position="append" only seeks to EOF at open time, concurrent writes from multiple ranks to the same physical file could race, causing corrupted/interleaved output or spurious I/O errors. Adopt the same strategy already used by EAM (atm_comp_mct.F90, atm_comp_esmf.F90): only masterproc reassigns iulog and opens/owns the log file. All other ranks simply keep iulog at its module default (stdout), avoiding the shared-file race entirely. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…river Initial implementation the MCT-based coupled driver for Omega. Includes necessary CIME configuration files (buildnml, buildlib_cmake, config_component/config_compsets), omega_cxx2f_interface/omega_f2cxx_interface bridges, a complete ocn_comp_mct.F90, split ocnInit1/ocnInit2, a coupled ocnRun overload, and an ocnFinalize bridge. The purpose of this initial PR is to confirm we can pass data to and from the coupler across the Fortran ⟺ C++ bridge and apply this data to the Omega Forcing class. In this PR we are not concerned with fully functional coupling or scientifically validated results; those will come in follow up PRs over the next few weeks. Therefore out target deliverable for the PR is a wind-driven C-Case using Omega run through the coupled driver.
Fix the cross product in "Requirements relative to vertices" and remove the caution admonition questioning its handedness. The requirement had the two operands exchanged: cellsOnVertex(n, iVertex) lies counter-clockwise of edgesOnVertex(n, iVertex), not the reverse, so the outward-pointing vector is (x_edgesOnVertex(n) - x_iVertex) x (x_cellsOnVertex(n) - x_iVertex) MpasMeshConverter.x is the source of truth. orderVertexArrays sorts edgesOnVertex counter-clockwise, then builds cellsOnVertex(n) from edgesOnVertex(n), selecting cellsOnEdge(1) when the vertex is verticesOnEdge(1) and cellsOnEdge(2) otherwise. Combined with the edge orientation convention, both branches select the cell counter-clockwise of edge n. This agrees with the kite construction, which spans edgesOnVertex(n) and edgesOnVertex(n+1) about cellsOnVertex(n). Empirical confirmation: the sign of both candidate cross products was checked on every valid (iVertex, n) pair across spherical, planar periodic, planar culled, ocean, sea-ice and land-ice meshes, including a fresh planar_hex -> MpasMeshConverter.x -> MpasCellCuller.x chain built with mpas_tools 2.0.0. mesh (edge-v)x(cell-v) reversed kite (n, n+1) QU1920 converted 960 / 960 0 / 960 2.7e-08 QU1920 culled (land bdy) 643 / 643 0 / 643 2.9e-08 planar hex periodic 864 / 864 0 / 864 2.7e-16 doubly periodic 5km planar 12000 / 12000 0 4.3e-16 QU240 / QUwISC240 culled 44605 / 44605 0 2.1e-08 SOMA 32km 51912 / 51912 0 2.1e-08 Humboldt 3-30km land ice 12582 / 12582 0 2.5e-16 The last column is the relative error in kiteAreasOnVertex assuming kite n spans edges (n, n+1); assuming edges (n-1, n) gives 40-50% error on every mesh. Two meshes disagreed, seaice_QU60km_polar.nc and gis20km.210608.nc, but both carry mesh_spec = 0.0 and descend from legacy grid files rather than MpasMeshConverter.x. Every mesh with mesh_spec = "1.0" agrees, without exception. The figure images/dualMesh_kiteAreas.png was already correct and needed no change. Measuring it about iVertex gives edgesOnVertex at 28.9, 155.3 and 269.1 degrees, cellsOnVertex at 89.7, 211.1 and 329.5 degrees, and the three kiteAreasOnVertex arrows at 98.1, 214.2 and 338.8 degrees, so each cell lies counter-clockwise of the edge with the same index and each kite falls between edges n and n+1. Also state the kite explicitly as the quadrilateral bounded by iVertex, edgesOnVertex(n), cellsOnVertex(n) and edgesOnVertex(n+1), and replace the ambiguous verb "lead" with "counter-clockwise of", which reads the same way whether or not the reader traverses the vertex in the counter-clockwise direction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Add a double-precision R8Vert variable to the file IOTest writes, then read it back into a single-precision buffer and read the existing R4Vert back into a double-precision buffer. IO::readNDVar goes through PIOc_get_var/PIOc_get_vara, which fill the caller's buffer using the type of the variable in the file rather than the type of the buffer. Reading a double variable into a float buffer therefore writes eight bytes per element into four-byte slots, which overruns the buffer and corrupts the heap. This is why VertCoord::init() corrupts the heap in an OMEGA_SINGLE_PRECISION build: VertCoordMovementWeights is the only non-distributed real field read from the mesh file, and it is stored there in double precision. Both destination buffers are allocated at twice the length needed, with the unused half set to a guard value, so the defect is reported as wrong values and overwritten guards rather than as an out-of-bounds write. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
IO::readNDVar called PIOc_get_var and PIOc_get_vara, which pass PIO_NAT to SCORPIO and therefore fill the caller's buffer using the type the variable has in the file rather than the type of the buffer. Reading a double variable into a single-precision array wrote eight bytes per element into four-byte slots, overrunning the buffer. This corrupted the heap in every OMEGA_SINGLE_PRECISION build that brought up VertCoord. VertCoordMovementWeights is dimensioned on NVertLayers alone, making it the only non-distributed real field Omega reads from the mesh file, and it is stored there in double precision; the read overran its 60-element buffer by 240 bytes. The damage was silent and surfaced later as a segmentation fault in an unrelated read or an invalid free during teardown. Double-precision builds were unaffected because the two types have the same width there. Pass the type of the destination buffer to readNDVar and dispatch to the type-specific SCORPIO calls so the values are converted on read. In IOStream::readFieldData the type is set in the same switch that allocates the buffer, so the two cannot drift apart. Distributed reads already carry this information in the decomposition and are unchanged. Also fixes the frame-time read in writeStream, which read the file's time variable into an R8 and would have produced garbage for a stream written with reduced precision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Register VERTCOORD_SINGLE_PRECISION_TEST, a single-precision build of the existing VertCoord test driver, following the pattern of TEND_PLANE_SINGLE_PRECISION_TEST. This is the coverage the readNDVar fix makes possible: VertCoord::init() reads the mesh file, which was corrupting the heap in any single-precision build, so no single-precision test could bring up a vertical coordinate. The checks were written as a fixed 1e-10 absolute tolerance, which single precision cannot meet. The expected values reach a few thousand once cell and layer indices accumulate down a column, and the sea surface height and target thicknesses are arrived at by cancelling contributions from every layer, so their error is set by the magnitude of the column rather than by the size of the result. Replace the hand-rolled comparisons with isApprox() from OceanTestCommon.h and a relative tolerance plus absolute floor, chosen per precision. Double precision is unchanged: with RTol = 0 and ATol = 1e-10, isApprox() reduces to exactly the comparison that was there before. The one difference is that isApprox() rejects NaN, which the old form silently accepted since a NaN difference is not greater than the tolerance. Also correct "Err += Err + 1" to "Err += 1" in the mid-pressure check. That doubling only affected the reported count, not pass or fail, but it overflows a signed int after about thirty failures, which single precision makes reachable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
prim_init_grid_views (called in prim_complete_init1_phase_f90, before dss_hvtensor) copies tensorVisc to C++ along with the other, constant geometry fields (D, Dinv, fcor, spheremp, rspheremp, metdet, metinv, vec_sph2cart, sphere_cart/latlon). But dss_hvtensor (called later, in prim_init_model_f90) updates elem(:)%tensorVisc afterwards, so the C++ copy of tensorVisc is stale by the time the model actually runs. This adds a new, narrow prim_init_tensorvisc subroutine (backed by a new init_tensorvisc_c/ElementsGeometry::set_tensorvisc C++ path) that re-copies just tensorVisc to C++ right after dss_hvtensor runs, without touching (or requiring recomputation of) the other geometry fields, which are constant and are still copied once, early, by the existing prim_init_grid_views call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#8591) EAMxx: add Moab in flight Adding the path for MOAB on Flight (a Sandia machine). [BFB]
Using Ekat yaml reader. Fixing photo table test. Fixing compilation error.
…t#8603) Remove unused deprecated py import in ww3 buildlib_cmake [BFB]
…ingle-precision IO::readNDVar filled the caller's buffer using the data type of the variable as stored in the file rather than the type of the buffer itself. Reading a double variable into a single-precision array wrote eight bytes per element into four-byte slots, overrunning the buffer and corrupting the heap. This affected every OMEGA_SINGLE_PRECISION build that initialized VertCoord and blocked any single-precision test needing a vertical coordinate. Pass the destination type to readNDVar so the values are converted on read, and add a single-precision VertCoord test now that one can run.
Add v3.2 configurations to testing suites Add v3.2 BluePulse configurations to the overnight testing suites. [BFB] for all currently tested configurations
…t#8599) This adds a missing temperature tendency in ZMDeepConvection::run_impl associated with the freezing of detrained condensate. This mirrors a term in EAM/CLUBB that was missed during the port. [non-BFB] - only when ZM is active
Previously, every MPI rank independently opened the same homme_log_fname file (root with status='REPLACE', all others with status='OLD'), each getting its own file description/offset. Since position="append" only seeks to EOF at open time, concurrent writes from multiple ranks to the same physical file could race, causing corrupted/interleaved output or spurious I/O errors. Adopt the same strategy already used by EAM (atm_comp_mct.F90, atm_comp_esmf.F90): only masterproc reassigns iulog and opens/owns the log file. All other ranks simply keep iulog at its module default (stdout), avoiding the shared-file race entirely. [BFB]
Replace the isort and flake8 pre-commit hooks with ruff-check and ruff-format, configured in components/omega/ruff.toml in a similar way to Polaris. Since python_lint.cfg no longer holds anything but the mypy settings, rename it to mypy.cfg. Update dev-conda.txt and the linting dev guide to match. Unlike Polaris, keep ruff-format's default double quote style, since that is what Omega's python already uses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Run ruff-check and ruff-format over the existing python files so that the new hooks pass on a clean tree. These changes are purely cosmetic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A few guardrails for EAM This PR contains three modifications that help prevent sensitivity experiments from violating model assumptions or getting into crashes. - Abort a simulation if hybi(1)/=0. - Initialize a few key variables with zero for radiation. - Skip tropopause search when a simulation has a low model top (i.e., all pmid values are > 450 hPa). These only affect certain non-standard EAM simulations (both global and single-column runs). [BFB]
…e-fix There seems to have been a minor mistake in resolving the diffs from E3SM-Project#521, which resulted in the e3sm_omega_developer test suite definition missing it's closing bracket.
…o-forcing This PR adds the thermo coupling - from forcing terms to thickness and tracer tendencies. The energy of mass fluxes and phase changes are hard-coded into the tendency conversions, like it was in MPAS-O. Hopefully the documentation (inline or otherwise) clarifies the meaning of each term. This PR is 3/3 to make E3SM-Project#418 more digestible.
…i_cpl_indices.F,seq_flds_mod.F90
add index_x2i_Fioo_frazilh (frazil heat flux from ocean) add index_x2i_Fioi_frazil (frazil mass flux from sea ice) add index_x2i_Fioi_frazils (frazil salt flux from sea ice) add index_x2i_Fioi_frazilh (frazil heat flux from sea ice) All added to mpassi_cpl_indices.F and seq_flds_mod.F90. Fioi_frazil and Fioi_frazils added to ice_comp_mct.F
Updates Icepack submodule to frazil coupling branch Adds ocean-ice coupling fields: frazil salt flux (Fioo_frazils) frazil enthalpy flux (Fioo_frazilh) defined consistent with freezingMeltPotential To use, set config_frazil_coupling_type to ‘omega-fluxes’ Backwards compatible with mpas-ocean for config_frazil_coupling_type = ‘external’ Stealth
Sea ice initialize now allows for the config_frazil_coupling_type option: "omega-fluxes"
Sea ice initialize NOW allows for config_frazil_coupling_type option: “omega-fluxes”
Removes double counting of frazil energy in coupler budget. Corrects possible but in temperature tendency of frazil melt. nBFB
Adds optional formulation in frazil.
Revert to original formulation with comments.
1. Initialized frazilMassFlux to 0 for SOM 2. Corrected indexing of component-level salt budget sum 3. Type in comment 4. Corrected definition of Fioo_frazils in MOAB to be consistent with mct
There was a problem hiding this comment.
🔵 Needs a closer look
The diff spans many unrelated subsystems (including a global default-driver change) beyond the stated frazil-coupling fix, increasing integration risk without a clearly scoped rationale.
Pull request overview
This PR aims to fix double counting of ocean heat flux during frazil ice
formation by splitting ocean→ice coupler heat terms (melt potential vs frazil
formation heat) and updating coupling behavior/configuration accordingly. In
addition, it includes a broad set of infrastructure updates across I/O,
tooling, and several component build/test trees.
Changes:
- Adjust ocean→ice coupling heat fields so
Fioo_qrepresents melt potential
only whileFioo_frazilhcarries frazil-formation heat, with recombination
handled on the ice side. - Add/extend I/O and timing support (e.g., HDF5/HDF5C iotypes, GPTL memusage
interface widening). - Add/refresh various build/test/documentation scaffolding across components
(notably EAMxx/HOMME/OMEGA/CIME).
File summaries
| File | Description |
|---|---|
run_e3sm.template.sh |
Adds an extra preview_namelists invocation for branch/hybrid runs; also contains a logging inconsistency called out in review. |
cime_config/customize/config.py |
Changes the global default driver selection to moab (scope/behavior change not reflected in PR description). |
components/eamxx/src/share/core/tests/eamxx_fpe_check.cpp |
Adds an FPE check test; minor correctness/clarity fixes suggested (literal types + namespace comment). |
share/util/shr_pio_mod.F90 |
Adds mapping for HDF5C PIO typename to pio_iotype_hdf5c. |
components/mpas-framework/src/driver/mpas_subdriver.F |
Extends MPAS mesh iotype selection to include hdf5 and hdf5c. |
share/timing/gptl.h |
Widens GPTLget_memusage signature to long long* to match implementation/usage patterns. |
share/timing/CMakeLists.txt |
Adds HAVE_SLASHPROC define on Linux for timing build configuration. |
Review details
- Files reviewed: 222/2665 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
33
to
35
| test_custom_project_machine = "mappy" | ||
| driver_default = "mct" | ||
| driver_default = "moab" | ||
| driver_choices = ("mct", "moab") |
Comment on lines
544
to
+548
| @@ -545,6 +545,7 @@ runtime_options() { | |||
| echo '$RUN_REFDIR = '${RUN_REFDIR} | |||
| echo '$RUN_REFCASE = '${RUN_REFCASE} | |||
| echo '$RUN_REFDATE = '${START_DATE} | |||
| ./preview_namelists | |||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Updated version of #535
Fixes double counting of ocean heat flux during frazil ice formation.
For the ocean to ice coupler fields, Fioo_q now only contains the melt potential while Fioo_frazilh contains the heat from frazil ice formation. frazilh and q are combined in ice_comp_mct to reproduce the sea ice freezingMeltingPotential
Fioo_q is removed from driver-mct as it is no longer a heat flux.
A new flag, config_use_accumulated_frazil_heat default false, is added to the ocean model. Default retains the latent heat definition frazil heat.
config_use_accumulated_frazil_heat = true uses the accumulatedFrazilHeat variable from frazil formation.
Tested in XS_2x5 GMPAS runs in 2 configurations:
and
Both completed successfully.
Test 1 was compared with darin's codebase from the previous PR (darincomeau@ea24e0f). Restart files at the end of day 11 showed differences in only 4 fields: o2x_ox_Fioo_frazilh, budg_dataG, o2x_ox_Fioo_q, and freezingMeltingPotential.
Differences in o2x_ox_Fioo_frazilh and freezingMeltingPotential were round off level only.
Differences in o2x_ox_Fioo_q, were all negative, as expected.
Differences in budg_dataG, also expected, were significant in regions of frazil ice formation.
Part of the frazil implementation documentation (sections 1.3-1.5). Currently here: https://www.overleaf.com/project/68896a760125362ca62587d1