EAMxx: Removes all fortran from C++ SHOC #6779

jgfouca · 2024-11-25T21:24:32Z

Following the pattern of a recent P3 PR, this PR changes the bfb unit tests to be based on baseline files instead of fortran calls. Unlike in that PR, we are actually able to completely remove all dependence on fortran code here because SHOC doesn't need any f90 init routines.

Change list:

Main change: For all the BFB SHOC unit tests, use baseline files instead of calling fortran!
Remove all of the CXX->f90 bridge code
Cleanup of shoc_init situation. SHOC has no global data, so you only need to call it if you need to compute npbl
Reorg by moving some less-interesting testing stuff into shoc/tests/infra. Now, the files presented in the public interface (just shoc directory) is a lot cleaner.
All shoc tests (bfb and run_and_cmp) accept the same set of arguments (-c for compare, -g for generate, -b for baseline dir, and -n for no baselines).
Some of the tests that just confirm that DIFFs are caught do need to do thread count spreads.
Remove the ability of shoc.F90 to call CXX impls..
gen-boiler can now generate _host functions for function that have array data.
No longer need to worry about transposing array data, we never cross language boundary anymore.
There were a lot of f90 functions that had a property test but were never actually used in the CXX. I removed all of these:

shoc_impli_sfc_fluxes_tests.cpp
shoc_impli_srf_stress_tests.cpp
shoc_impli_srf_tke_tests.cpp
shoc_energy_total_fixer_tests.cpp
shoc_energy_dse_fixer_tests.cpp
shoc_energy_threshold_fixer_tests.cpp
shoc_fterm_input_third_moms_tests.cpp
shoc_fterm_diag_third_moms_tests.cpp
shoc_omega_diag_third_moms_tests.cpp
shoc_xy_diag_third_moms_tests.cpp
shoc_aa_diag_third_moms_tests.cpp
shoc_w3_diag_third_moms_tests.cpp

[BFB]

Moved over from: E3SM-Project/scream#3121

P3 DIFFs are expected because I caught a few places that were transposing views that didn't need to do that anymore. SHOC fails are expected because the baselines haven't been generated for the containers yet.

Follow on work:

Port P3 init code from fortran; this is the last of the fortran being used by either p3 or shoc
Remove all references to "fortran" or "Fortran". These names are all obsolete.

…_f90_shoc * origin/master: (326 commits) EAMxx: Add aerosols and gases outputs to ensure the microscopy baseline test is sensitive to changes in the interface. fix standard names for low/med/hgh cloud area fraction types Add standard name metadata to most EAMxx output variables EAMxx: Update mam4xx submodule to fix CUDA test. EAMxx: Modify microphysics interface to include sethet (washout rates) computations. Bump DavidAnson/markdownlint-cli2-action from 17 to 18 EAMxx: Moves fences into the interface and remove extra comments EAMxx: Adds missing namelist entries for multi process test EAMxx: Remove all diffs from microphysics cpp file ff mam4xx to current main Revert "EAMxx: Clang format" EAMxx:Inits constituent fluxes to zero fix cuda compile warnings EAMxx: Moves online emiss and constituent init to a new function hpp file EAMxx: Fixed some comments paths and other minor cleanup EAMxx: Clang format EAMxx: Fixes a gpu bug requiring a particular format for dstflx update mam4xx submodule to fix gpu compilation error EAMxx:Removes accidently added file EAMxx: Some codes re-arranged after rebase ...

github-actions · 2024-11-25T21:27:07Z

PR Preview Action v1.4.8
🚀 Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6779/
on branch `gh-pages` at 2024-11-29 21:01 UTC

bartgol · 2024-12-02T22:11:18Z

components/eamxx/src/physics/shoc/tests/infra/shoc_ic_cases.cpp

-using Array1 = typename KT::template lview<Scalar*>;
-using Array2 = typename KT::template lview<Scalar**>;
-using Array3 = typename KT::template lview<Scalar***>;
+using Array1 = typename FortranData::Array1;


What's the meaning of "FortranData" here? Should this be maybe called something like "BaselineData" or is there still some Fortran reminescence?

@bartgol , I made note of those in the follow-on work. These will be renamed but i don't think it's worth doing another CI run on this PR.

Following the pattern of a recent P3 PR, this PR changes the bfb unit tests to be based on baseline files instead of fortran calls. Unlike in that PR, we are actually able to completely remove all dependence on fortran code here because SHOC doesn't need any f90 init routines. Change list: 1) Main change: For all the BFB SHOC unit tests, use baseline files instead of calling fortran! 2) Remove all of the CXX->f90 bridge code 3) Cleanup of shoc_init situation. SHOC has no global data, so you only need to call it if you need to compute npbl 4) Reorg by moving some less-interesting testing stuff into shoc/tests/infra. Now, the files presented in the public interface (just shoc directory) is a lot cleaner. 5) All shoc tests (bfb and run_and_cmp) accept the same set of arguments (-c for compare, -g for generate, -b for baseline dir, and -n for no baselines). 6) Some of the tests that just confirm that DIFFs are caught do need to do thread count spreads. 7) Remove the ability of shoc.F90 to call CXX impls.. 8) gen-boiler can now generate _host functions for function that have array data. 9) No longer need to worry about transposing array data, we never cross language boundary anymore. There were a lot of f90 functions that had a property test but were never actually used in the CXX. I removed all of these: * shoc_impli_sfc_fluxes_tests.cpp * shoc_impli_srf_stress_tests.cpp * shoc_impli_srf_tke_tests.cpp * shoc_energy_total_fixer_tests.cpp * shoc_energy_dse_fixer_tests.cpp * shoc_energy_threshold_fixer_tests.cpp * shoc_fterm_input_third_moms_tests.cpp * shoc_fterm_diag_third_moms_tests.cpp * shoc_omega_diag_third_moms_tests.cpp * shoc_xy_diag_third_moms_tests.cpp * shoc_aa_diag_third_moms_tests.cpp * shoc_w3_diag_third_moms_tests.cpp [BFB] Moved over from: E3SM-Project/scream#3121 P3 DIFFs are expected because I caught a few places that were transposing views that didn't need to do that anymore. SHOC fails are expected because the baselines haven't been generated for the containers yet. Follow on work: Port P3 init code from fortran; this is the last of the fortran being used by either p3 or shoc Remove all references to "fortran" or "Fortran". These names are all obsolete.

rljacob · 2024-12-05T06:32:18Z

This should have been tested overnight on next before merging to mater since it touched code outside of eamxx.

bartgol · 2024-12-05T17:05:14Z

This should have been tested overnight on next before merging to mater since it touched code outside of eamxx.

You're right that it touches one file outside of eamxx, but I think this is somewhat of a special case. All the code removed from shoc.F90 was code guarded by ifdef SCREAM_CONFIG_IS_CMAKE, which is only defined if eamxx is built. All EAM cases (including MMF) should NOT be affected by that.

whannah1 · 2024-12-05T17:10:14Z

@bartgol

> grep "SCREAM_CONFIG_IS_CMAKE" components/eam/src/physics/crm/pam/* -r
components/eam/src/physics/crm/pam/external/physics/scream_cxx_interfaces/CMakeLists.txt:add_definitions(-DSCREAM_CIME_BUILD -DHAVE_MPI -DSCREAM_CONFIG_IS_CMAKE)

bartgol · 2024-12-05T17:21:36Z

@bartgol

> grep "SCREAM_CONFIG_IS_CMAKE" components/eam/src/physics/crm/pam/* -r
components/eam/src/physics/crm/pam/external/physics/scream_cxx_interfaces/CMakeLists.txt:add_definitions(-DSCREAM_CIME_BUILD -DHAVE_MPI -DSCREAM_CONFIG_IS_CMAKE)

Ah, crap, I only ran git grep without --recurse-submodules... One of the reasons why I sometimes don't like submods.

whannah1 · 2024-12-05T17:30:33Z

yea - and there's a list of reasons why pulling PAM into the repo makes sense. I hope to do that in the next year or so, but it depends on what we do with YAKL, among other things.

bartgol · 2024-12-05T17:37:32Z

That said, I think the code that Jim purged from shoc.F90 is ONLY needed for eamxx's ability to call f90 implementation as part of the CXX-vs-F90 bfb testing. If PAM uses scream cxx impl, it should still be able to build with this PR merged.

whannah1 · 2024-12-05T17:44:25Z

@bartgol yea, it's an easy fix for PAM to remove the include statements, I created a PR yesterday.

rljacob · 2024-12-05T20:03:19Z

@bartgol your request for an exception in the future is denied.

mahf708 · 2024-12-05T20:08:04Z

This should have been tested overnight on next before merging to mater since it touched code outside of eamxx.

I think this is a problem for how MMF (especially PAM) logic is constructed. I would be hesitant to make this into an EAMxx responsibility. I think PAM should be reworked to rely on EAMxx code more cleanly and abstractly. It's not designed well imo

mahf708 · 2024-12-05T20:09:00Z

This keeps happening ...

mahf708 · 2024-12-05T20:10:16Z

The outside code in this PR is really eamxx code. The issue is that PAM calls stuff way in the weeds, so we end up breaking it with every little touch. This is a PAM problem

For example, MMF is pulling stuff that it really doesn't need to pull and it is unclear if it ever needed it... 👀

bartgol · 2024-12-05T21:04:31Z

@bartgol your request for an exception in the future is denied.

Tbc, I wasn't requesting exceptions. I was just pointing out that the collateral damage in this case should be minimal.

jgfouca added 26 commits November 11, 2024 13:13

progress

193e5fa

Add sentinel for parsing

9820033

This file had windows carraige returns

bea3c66

prog

33d6d3f

prog

6a92084

prog

940af13

Use data funcs for CXX now

937850e

builds

b7e95cf

progress

cb6471f

progress

0b56156

shoc_pdf_vv_parameters

9a0e9a6

shoc_pdf_thl_parameters_property

deb2d7d

shoc_pdf_qw_parameters_property

573cb41

shoc_pdf_inplume_corr_property

081bec9

shoc_assumed_pdf_compute_temperature

524fd71

shoc_assumed_pdf_compute_qs

2c973d8

shoc_assumed_pdf_compute_s

504d774

shoc_assumed_pdf_compute_sgs_liquid

52d9d69

shoc_assumed_pdf_compute_cloud_liquid_variance

1878caf

shoc_assumed_pdf_compute_liquid_water_flux

ffd8ae1

shoc_assumed_pdf_compute_buoyancy_flux

8ac353c

progress

3cacc63

progress

59f4fb0

builds

bee699d

Fix baseline read

444bc1f

jgfouca added BFB PR leaves answers BFB EAMxx C++ based E3SM atmosphere model (aka SCREAM) labels Nov 25, 2024

jgfouca requested review from AaronDonahue and bartgol November 25, 2024 21:24

jgfouca requested review from bogensch and tcclevenger November 25, 2024 21:24

jgfouca self-assigned this Nov 25, 2024

bartgol approved these changes Nov 25, 2024

View reviewed changes

bogensch approved these changes Nov 25, 2024

View reviewed changes

jgfouca added 2 commits November 25, 2024 16:16

minor fixes

c522aad

FortranData should no longer be using lviews

f1bc5d6

tcclevenger approved these changes Nov 29, 2024

View reviewed changes

Fixes for minor test mistakes

c4829c4

AaronDonahue changed the title ~~Removes all fortran from SHOC~~ EAMxx: Removes all fortran from SHOC Dec 2, 2024

bartgol reviewed Dec 2, 2024

View reviewed changes

jgfouca merged commit 8f12c3f into master Dec 3, 2024
14 of 21 checks passed

jgfouca deleted the jgfouca/bfb_unit_no_f90_shoc branch December 3, 2024 18:16

rljacob changed the title ~~EAMxx: Removes all fortran from SHOC~~ EAMxx: Removes all fortran from C++ SHOC Dec 5, 2024

rljacob mentioned this pull request Dec 5, 2024

update PAM submodule to fix build error #6801

Merged

mahf708 mentioned this pull request Dec 6, 2024

Removes all fortran from SHOC E3SM-Project/scream#3121

Closed

EAMxx: Removes all fortran from C++ SHOC #6779

EAMxx: Removes all fortran from C++ SHOC #6779

Uh oh!

Conversation

jgfouca commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bartgol Dec 2, 2024

Choose a reason for hiding this comment

Uh oh!

jgfouca Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rljacob commented Dec 5, 2024

Uh oh!

bartgol commented Dec 5, 2024

Uh oh!

whannah1 commented Dec 5, 2024

Uh oh!

bartgol commented Dec 5, 2024

Uh oh!

whannah1 commented Dec 5, 2024

Uh oh!

bartgol commented Dec 5, 2024

Uh oh!

whannah1 commented Dec 5, 2024

Uh oh!

rljacob commented Dec 5, 2024

Uh oh!

mahf708 commented Dec 5, 2024

Uh oh!

mahf708 commented Dec 5, 2024

Uh oh!

mahf708 commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bartgol commented Dec 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jgfouca commented Nov 25, 2024 •

edited

Loading

github-actions bot commented Nov 25, 2024 •

edited

Loading

mahf708 commented Dec 5, 2024 •

edited

Loading