Removes all fortran from SHOC #3121
Conversation
…_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 ...
jgfouca
requested review from
bartgol,
PeterCaldwell,
AaronDonahue and
tcclevenger
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 Enforce checks passingThis rule is failing.Make sure that checks are not failing on the PR, and reviewers approved
|
| } | ||
| shoc_use_cxx_c(!use_fortran); | ||
| } | ||
|
|
||
| int test_FortranData () { |
There was a problem hiding this comment.
Does this mean that even thought we've dropped F90 support we still need to use some data in the Fortran structure somewhere? Or just test for it?
There was a problem hiding this comment.
Perhaps it can be done as a follow up PR, but I would also argue in favor of pruning any reference to Fortran, like FortranData and FortranDataIterator. If they are still needed, we should rename into something like HostData or BaselineData, or just TestData...
There was a problem hiding this comment.
I skimmed this and it looks good to me. I restricted my review to things outside of src/physics/shoc/tests (so ignoring the vast majority of the PR) as I won't be of much help for the tests themselves. Everything outside of the tests looks good, and so I am approving!
Heroic effort at record speed 🔥
| shoc_main_wrap.cpp | ||
| ) # Add f90 bridges needed for testing | ||
| endif() | ||
|
|
||
| set(SHOC_HEADERS | ||
| shoc.hpp |
There was a problem hiding this comment.
This file does not even exist. Let's prune this line.
| } | ||
| shoc_use_cxx_c(!use_fortran); | ||
| } | ||
|
|
||
| int test_FortranData () { |
There was a problem hiding this comment.
Perhaps it can be done as a follow up PR, but I would also argue in favor of pruning any reference to Fortran, like FortranData and FortranDataIterator. If they are still needed, we should rename into something like HostData or BaselineData, or just TestData...
| @@ -67,17 +67,13 @@ struct FortranDataIterator { | |||
| void init(const FortranData::Ptr& d); | |||
| }; | |||
|
|
|||
| // Initialize SHOC with the given number of levels. | |||
| void shoc_init(Int nlev, bool use_fortran=false, bool force_reinit=false); | |||
|
|
|||
| // We will likely want to remove these checks in the future, as we're not tied | |||
| // to the exact implementation or arithmetic in SHOC. For now, these checks are | |||
| // here to establish that the initial regression-testing code gives results that | |||
| // match the python f2py tester, without needing a data file. | |||
| Int check_against_python(const FortranData& d); | |||
There was a problem hiding this comment.
It appears that this function (like the one with the same name in p3) is never defined. Let's prune this decl too.
| @@ -124,11 +121,11 @@ struct UnitWrap::UnitTest<D>::TestCompBruntShocLength { | |||
| } | |||
| } | |||
|
|
|||
| static void run_bfb() | |||
There was a problem hiding this comment.
What was the reason for making the run methods non-static? Not that I have a preference in case both are fine, I don't see any pros/cons. Just curious.
| } | ||
| } | ||
| } // SCREAM_BFB_TESTING | ||
| else if (this->m_baseline_action == GENERATE) { |
There was a problem hiding this comment.
I comment here, though it's a more general question.
Why do we want to give the possibility of not generating and not comparing? What's the use case for the -n flag of the tests?
|
E3SM-Project/E3SM#6779 was merged, so closing |
…ect#6779) 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.
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:
npbl