Reimplement smartredis #159
Conversation
|
This is a draft PR for now since I need to make sure that the implementation still works. I would appreciate feedback from @adcroft and @marshallward on the overall design to isolate the portions of the codebase that depend on the external SmartRedis package. @adcroft, I think this largely follows the strategy that we had outlined. |
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #159 +/- ##
============================================
+ Coverage 37.40% 37.52% +0.11%
============================================
Files 259 261 +2
Lines 71840 71748 -92
Branches 13442 13456 +14
============================================
+ Hits 26871 26921 +50
+ Misses 40025 39836 -189
- Partials 4944 4991 +47
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
ashao
force-pushed
the
reimplement_smartredis
branch
4 times, most recently
from
ff5fcf9 to
4960fff
Compare
|
For some reason, Doxygen objects to the I don't even want to understand this problem. But replacing with |
ashao
force-pushed
the
reimplement_smartredis
branch
from
4960fff to
aea6fd9
Compare
ashao
force-pushed
the
reimplement_smartredis
branch
3 times, most recently
from
eb6321b to
bbb7ad7
Compare
ashao
force-pushed
the
reimplement_smartredis
branch
4 times, most recently
from
2acfff1 to
d60cdd2
Compare
Eddy kinetic energy can now be read in from a time-varying field in a file. The interpolated EKE is used in place of MEKE's PDE based approximation. All the other options that are used to estimate viscosity, GM coefficient, and tracer eddy diffusivity are still valid. Note: this feature has not been extensively tested.
Significant changes to MOM_MEKE.F90 made it difficult to direcly merge the initial implementation of SmartRedis with MOM6. A refactor of the implementation was also done in consultation with @adcroft to isolate the SmartRedis implementations of the code which includes creating two stub modules for the - SmartRedis client module with associated methods - MEKE-related module for inferring EKE using a neural network via SmartRedis The substantive code is now hosted for users interested in replicating the work at https://github.com/CrayLabs/MOM6-smartredis
The implementation of inferring EKE using a neural network via a machine-learning interface has been further refactored to: - Isolate the mention of specific solutions, instead referring to a name that is more descriptive of its functionality (i.e. dbclient instead of smartredis) - The calculation of the features is also now included in the main MOM6 codebase
The changes here remove all references to specific implementations of clients used to communicate with the database. Additionally, references within MOM6 now refer "MOM_database_comms" as the module name (with similarly named methods) versus the dblcient_type. Packages are now expected to provide the following implementations which are compatible with those found in config_src/external/database_comms/MOM_database_comms.F90: - dbclient_type - dbcomms_CS_type - subroutine database_comms_init
ashao
force-pushed
the
reimplement_smartredis
branch
from
2f8f831 to
e777f1f
Compare
There was a problem hiding this comment.
The SmartRedis communication layer has been well-abstracted and there is a strong logical separation between the the DB and the MEKE solver code. Overall I think this is a very good design and a good template for future external integration.
There are some minor issues related to the use of put_tensor() which accepts C types (specifically c_float) but the current equivalent with real32 is reasonably safe and we can address this in the future if the need arises.
The stub code for database_client_interface still contained references to iso_c_binding. This removes mention from it there instead requiring that various types are part of iso_fortran_env. Implementations of the methods and types by specific packages must comply with these specific types through a wrapper or directly.
…eimplement_smartredis
Doxygen is wrongly flagging that the dummy methods in the database client stub are undocumented. This may have been related to a different change during the previous refactor of this stub. The solution for now is to simple move the docstring to precede the function
|
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/16294 ❌ This is not passing our regression, due to assumed-rank in Since MOM6 is nominally a Fortran 2003/2008 program, these will need to be replaced with something older and clunkier. |
ashao
force-pushed
the
reimplement_smartredis
branch
from
3d3d326 to
25ac027
Compare
The database client stub used Fortran 2018 features which do not adhere to the MOM6 allowing only up to Fortran 2008. To fix this, separate interfaces for tensors with 1-4 dimensions replace the assumed rank subroutines. This does not interfere with the SmartRedis library as the public (overloaded) interface still retains the same API. Additionally, some methods which are likely unique to the SmartRedis client have been removed to enhance the likelihood of providing a general database client API to be used by other implementations.
…mplement_smartredis
ashao
force-pushed
the
reimplement_smartredis
branch
from
0a2baca to
3b006e0
Compare
|
Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/16346 ✔️ 🟡 |
Significant changes to MOM_MEKE.F90 made it difficult to direcly
merge the initial implementation of SmartRedis with MOM6. A
refactor of the implementation was also done in consultation with
@adcroft to isolate the SmartRedis implementations of the code
which includes creating two stub modules for the
via SmartRedis
The substantive code is now hosted for users interested in
replicating the work at https://github.com/CrayLabs/MOM6-smartredis