Convert p3_init from f90 to CXX #6845
Conversation
This removes the last f90 from P3. Change list: * Move table read/write implementation to CXX in p3_init_impl.hpp * p3_init now returns a P3LookupTables object with all the views inside containing the tables. * Rename init_kokkos_tables to get_global_tables * Rename init_kokkos_ice_lookup_tables to get_global_ice_lookup_tables * Remove all f90 bridge code * p3_tables_setup is now built by default and ran when baseline gen is on. This gives us the ability to generate these tables through GH actions which will be needed for this PR. * Move p3_tables_setup to tests dir, cleans up top p3 directory * Fix p3_tables_setup implementation; it got broken in recent PRs but we didn't notice because it didn't get built by default * Remove P3GlobalForFortran test struct, just call p3_init! * Remove P3InitAP3Data test struct Debatable change: I really wanted to have `P3LookupTables lookup_tables` be static in p3_init so they would not have to be re-created on every p3_init call. Unfortunately, Kokkos does not allow static views to be active on program shutdown, since this occurs after Kokkos::finalize, so we would need add a p3_finalize method and ensure it was called at the end of p3. This seems burdensome and error prone, so I just decided to re-read all the tables every time p3_init is called. [non-BFB]
jgfouca
requested review from
bartgol,
ambrad,
AaronDonahue,
tcclevenger and
mahf708
|
Reviewers, please see "Debatable change" section in the description. |
|
There was a problem hiding this comment.
NICE! I love PRs where the # of rm-ed lines is larger than the # of added lines...
I have a few suggestions, in case you want to take this a step further. Not blocking though.
As for the debatable change: I think leaving as is (init for every test) is fine. No need to add complexity just to make the tests a bit faster. Our tests are reasonably fast anyways, so unless they suddenly got slow, and need the speedup of a single read, I think it's fine.
| std::cout << "Reading ice lookup tables in file: " << filename << std::endl; | ||
| } | ||
|
|
||
| std::ifstream in(filename); |
There was a problem hiding this comment.
Does this mean that the tables are plain ascii now? I kind of like that, so one can always inspect the file content manually, and since they are relatively small and only used at init time, binary format is not that important.
There was a problem hiding this comment.
@bartgol , I should have made it more clear in the PR description that this function impl is, for the most part, not new. It's just what init_kokkos_ice_lookup_tables was before except moved here and renamed.
| EKAT_REQUIRE_MSG(version_val == p3_version, "Bad " << filename << ", expected version " << p3_version << ", but got " << version_val); | ||
|
|
||
| // read tables | ||
| double dum_s; int dum_i; // dum_s needs to be double to stream correctly |
There was a problem hiding this comment.
Are these just dummy vars to swallow entries that are in the file but not used? Do we need to keep these unused entries in the file or can we just prune them, and keep only what is used?
There was a problem hiding this comment.
It looks like dum_s is used but not dum_i. We probably don't need the unused entries, but I don't really know the history of this file.
| struct IoAction | ||
| { | ||
| template <typename S> | ||
| static void action(const ekat::FILEPtr& fid, S* data, const size_t size) |
There was a problem hiding this comment.
Up to you, but since we are doing C++ only, maybe we should just use <fstream> stuff, rather than the cumbersome FILEPtr? It seems easier to understand for new devs, and easier to maintain.
There was a problem hiding this comment.
I actually found the ekat IO stuff to be pretty useful, particularly for reading arrays.
| } | ||
|
|
||
| std::string extension = | ||
| #ifdef SCREAM_DOUBLE_PRECISION |
There was a problem hiding this comment.
Idea: since we are in the middle of rearranging our tables, why not being more verbose with names? Maybe instead of appending 8 or 4, we could append the actual type name? Like mu_r_table_vals_double.dat?
Yes, it will require changing defaults in the xml, but if clarity increases, it's worth it, imho.
There was a problem hiding this comment.
This is the convention we are using all over, for better or worse.
| } | ||
| }; | ||
|
|
||
| template <> |
There was a problem hiding this comment.
Rather than using template specialization, why not doing
static void action (...)
{
if constexpr (IsRead)
ekat::read (...);
else
ekat::write(...);
}
| @@ -113,10 +111,6 @@ foreach (file IN ITEMS ${P3_TABLES}) | |||
| GetInputFile(${file}) | |||
| endforeach() | |||
|
|
|||
There was a problem hiding this comment.
I can't add a comment out of the lines you modified, so I'm writing here.
Since we have NO fortran left, we should remove the lines
set_target_properties(${P3_LIB} PROPERTIES
Fortran_MODULE_DIRECTORY ${P3_LIB}/modules
)as well as the line ${CMAKE_CURRENT_BINARY_DIR}/${P3_LIB}/modules inside the target_include_directories call (we don't generate fortran .mod files anymore)
Simplify io action using if constexpr. Remove fortran module stuff from cmake Remove fortran from comment
|
@jgfouca Before merging this, we should decide how to handle tables. Once merged, we will need to regenerate the tables, right? But old e3sm versions will still need the "old" f90-major tables. Replacing the old files with the new will cause folks running old code to get diffs. I think we should change the tables names, so that we can have both old and new tables on the server. Maybe we should add v0.1 and v1.0 at the end of names, or something like that. |
|
@bartgol , crap, I did not think of that. Yes, this change probably requires a new table version... |
This removes the last f90 from P3.
Change list:
Debatable change:
I really wanted to have
P3LookupTables lookup_tablesbe static in p3_init so they would not have to be re-created on every p3_init call. Unfortunately, Kokkos does not allow static views to be active on program shutdown, since this occurs after Kokkos::finalize, so we would need add a p3_finalize method and ensure it was called at the end of p3. This seems burdensome and error prone, so I just decided to re-read all the tables every time p3_init is called.There are some round-off level diffs in P3, so:
[non-BFB]