Skip to content

[Docs]: issues from code review of PR 3896 #3898

Description

@asalmgren

Affected page or section

Documentation of WSM6/WDM6

What is unclear or incorrect?

Remaining findings

A. Medium — the GPU parity claim was deleted, not replaced

Docs/sphinx_doc/theory/Microphysics.rst:362-368

Commit 6fc6e3843 removed the false sentence, leaving only "A native C++
implementation of all WDM6 microphysical processes enables efficient execution on
GPUs." The page now makes no accuracy statement at all, while the Fortran path is
still described as one that "ensures reproducibility with WRF results". A reader
comparing the two paragraphs will still conclude the GPU path is validated.

Source/Microphysics/WDM6/README:44-58,200-256 records the opposite:

  • GPU bridge-vs-native divergence is "order-unity, not small" (worst 1.06
    relative on rhoQ4 at step 10, 0.43 on nr at step 100), confined to the
    warm-rain group
  • the comparison that would isolate the port — GPU-native against CPU-native —
    has not been run, and the README says so explicitly

A one-sentence pointer to that README, or a short note stating the CPU agreement
level and the open GPU question, closes this properly.

B. Medium — the misleading CPU/GPU framing survives in both Overview sections

Docs/sphinx_doc/theory/Microphysics.rst:228 and :340

Commit 98394a398 fixed the Configuration paragraphs ("The default build uses
native C++ code that can run on both CPU and GPU"), but the Overview and
Implementation text that finding 5 was about is unchanged:

  • :228 — "provides both CPU (via Fortran-C++ bridge) and GPU (native C++
    implementation) execution paths"
  • :340 — "supports both CPU (via Fortran-C++ bridge) and GPU (native C++
    implementation) execution"
  • :250-260 and :356-366 — the subsection headings CPU execution (Fortran
    bridge)
    and GPU execution (native C++)

The page therefore contradicts itself four lines apart: the default build has no
Fortran at all and runs the native C++ kernels on CPU, and the Fortran bridge
never runs on GPU. Relabelling the two paragraphs by what they are rather than
where they run — e.g. "Fortran bridge (optional, CPU only)" and "Native C++
(default, CPU and GPU)" — fixes both sections.

C. Medium — Inputs.rst still omits the two runtime switches

Docs/sphinx_doc/Inputs.rst:2074-2085

Added: "WSM6" / "WDM6" to the erf.moisture_model acceptable values,
wdm6.hail_opt, and wdm6.ccn0. Verified against
Source/Microphysics/WDM6/ERF_InitWDM6.cpp:23-33 and
Source/Microphysics/WDM6/ERF_WDM6.H:225 — prefix wdm6, default
100.0e6 m^-3, hail_opt default 0: all correct.

Still missing:

  • erf.use_wsm6_cpp_answer and erf.use_wdm6_cpp_answer appear nowhere in
    Docs/sphinx_doc/ outside the new theory page (git grep cpp_answer over
    Docs/ returns only Microphysics.rst:284,389 and an unrelated fix_me
    entry). They are user-typeable erf.* options
    (ERF_AdvanceWSM6.cpp:871, ERF_AdvanceWDM6.cpp:699) and belong in the input
    reference.
  • "SuperDroplets" is still absent from the erf.moisture_model acceptable
    values cell, which this PR edited.

D. Medium — nn is documented as a provider-generated name, but it is a fixed one

Docs/sphinx_doc/plotfiles/Plotfile3DReference.rst:845 and :664

The new WDM6 row in the scheme table (:473) is correct — verified against
Source/IO/ERF_PlotfileSelection.H:155-167 (vapor, cloud_liquid, cloud_ice,
rain, snow, graupel, cloud_number, rain_number, ccn_number, and all three
accumulations).

The second addition is in the wrong section. nn, nc, nr are fixed
plotfile names, resolved by plot3d_moist_* in
ERF_PlotfileSelection.H:344-351 and copied in ERF_Plotfile.cpp:1413,1427,1451.
They are not returned by a provider hook — WDM6 implements no plot-name hook at
all. Putting them under "Scheme-provided dynamic fields", whose premise is
"There is no universal static list for these fields", tells the reader the wrong
mechanism.

Meanwhile the bullet that does enumerate the fixed number-concentration names
(:664) still reads "nc, ni, nr, ns, and ng" — nn was not
added there, so the one place a user looks up a fixed name is still incomplete.
Fix: add nn at :664 and drop the :845 bullet.

E. Low — the new inputs file's header contradicts this PR and cites a test that does not exist

Exec/RegTests/Bubble/inputs_BF02_moist_bubble_wdm6:1-23

Two problems in the copied boilerplate (inherited from _water / _hail, but
newly added text in this PR):

  • "The base input is also shared with the nightly Bubble_WSM6 regression test and
    must not be edited" — this same PR edits that base input
    (inputs_BF02_moist_bubble), and grep -rn Bubble_WSM6 matches only these
    three inputs files. Tests/CTestList.cmake has no Bubble WSM6/WDM6 test; the
    only WSM6 entry is SHOC_Unstable_Cloud_WSM6 (:614).
  • "the campaign runner supplies the 32-entry list including nc/nn/nr" refers to
    a harness that is not in the repo. The arithmetic checks out (base
    plot_vars_1 has 29 entries, plus nc/nn/nr = 32), but the reference is
    unresolvable to anyone reading the file.

The mechanism itself is sound: AMReX ParmParse does support FILE = <name>
includes (ParmParse::FileKeyword, AMReX_ParmParse.cpp:44,774) and resolves a
repeated key to its last occurrence, so the delta-only construction works.

F. Low — the new CMake rows land inside a table that docutils rejects

Docs/sphinx_doc/buildingSystems.rst:585-588

The "ERF CMake Options" list-table has an orphaned, over-indented row:

   * - ``ERF_ENABLE_RRTMGP``
     - Enables RRTMGP radiation model (...)
     - OFF
     - ON/OFF
      * - Native SHOC          <-- over-indented, no parent row
        - Builds the in-tree SHOC path ...

docutils reports Error parsing content block for the "list-table" directive: two-level bullet list expected, but row 14 does not contain a second-level bullet list. This is pre-existingdevelopment produces the identical error at
the same row, so the PR did not cause it. It matters here only because the new
ERF_ENABLE_WSM6_FORT / ERF_ENABLE_WDM6_FORT rows are in that table, after the
malformed row, so finding 6's fix may not render in the built docs. A four-line
drive-by dedent (or deletion of the stray row) would make it visible. Worth
confirming against an actual Sphinx build.

G. Low — Morrison rows in the same table use the wrong case

Docs/sphinx_doc/plotfiles/Plotfile3DReference.rst:465-472

The Morrison rows list Nc, Nr and Nc, Ni, Nr, Ns, Ng, but the selectable
names are lowercase (ERF_PlotfileSelection.H:344-351, and :664 in this same
document). The new WDM6 row uses the correct lowercase form, which makes the
neighbouring rows read as if the case were meaningful. Pre-existing; cheap to fix
while the table is being touched.

H. Low — the WSM6 "Example Cases" list omits the only automated WSM6 test

Docs/sphinx_doc/theory/Microphysics.rst:299-303

The list leads with an input file that requires the reader to hand-edit
erf.moisture_model, and does not mention
Tests/test_files/SHOC_Unstable_Cloud/SHOC_Unstable_Cloud_WSM6.i, which is wired
into CTest (Tests/CTestList.cmake:614-619) and therefore the one configuration
guaranteed to work. None of the four Bubble WSM6/WDM6 inputs is registered in
CTestList.cmake.

Suggested improvement

No response

Metadata

Metadata

Assignees

No one assigned

    Labels

    documentationImprovements or additions to documentation

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions