Download crtm coefficient once and use it by crtm, ufo, fv3-jedi and mpas-jedi#302
Download crtm coefficient once and use it by crtm, ufo, fv3-jedi and mpas-jedi#302rajichidamb wants to merge 5 commits into
Conversation
…erty so UFO can use the same path to get the crtm coefficients
| endif() | ||
|
|
||
|
|
||
| set (UFO_CRTM_TESTFILES_PATH ${CRTM_COEFFS_PATH}/${CRTM_COEFFS_BRANCH}/fix) |
There was a problem hiding this comment.
CRTM is often used outside of JEDI. I wonder if it made sense to create a CMake option that tells the build system if this is a build in JEDI or standalone, and then set stuff like this. That might also help with the compiler flags discussion that @fmahebert and @BenjaminTJohnson had.
There was a problem hiding this comment.
How about naming this CRTM_TESTFILES_PATH ?
There's no need to prefix it with UFO. This way, it stays neutral and my comment from this morning can be ignored. Same in the UFO PR.
There was a problem hiding this comment.
Is the variable CRTM_COEFFS_PATH available when the if (EXISTS ${FIX_FILES_PATH}) code path is taken above (instead of the download code path) ?
There was a problem hiding this comment.
I was following the else code of (EXISTS ${FIX_FILE_PATH}), setting the CRTM_COEFFS_PATH.
| set( CRTM_COEFFS_BRANCH "fix_REL-3.1.2.0" ) | ||
|
|
||
| set(CRTM_COEFFS_PATH ${CMAKE_BINARY_DIR}/test_data/${PROJECT_VERSION}) | ||
| set(CRTM_COEFFS_PATH ${CMAKE_SOURCE_DIR}/test-data-release) |
There was a problem hiding this comment.
This is outside my expertise, perhaps better for @BenjaminTJohnson or @climbfuji — do we think using CMAKE_SOURCE_DIR will work well when CRTM is installed as an external library (vs as part of a compiled bundle), e.g. in the case of using spack-stack to provide a CRTM module?
Or is this question moot, because this is all in test/CMakeLists.txt, and this code path is probably not exercised in a CRTM-as-a-library context?
There was a problem hiding this comment.
This is actually a core part of the librification of CRTM and other JEDI components. We need to be able to run the ctests. I had to patch the CRTM code (and the UFO code) in order to make the stars line up. I can update the patch as needed in the Spack recipe, hence I don't see this as a big problem. The only thing I am adament about is to not prefix CRTM_TESTFILES_PATH with UFO_, because those testfiles are CRTM's testfiles and independent of whether UFO is being used or not?
| endif() | ||
|
|
||
|
|
||
| set (UFO_CRTM_TESTFILES_PATH ${CRTM_COEFFS_PATH}/${CRTM_COEFFS_BRANCH}/fix) |
There was a problem hiding this comment.
Is the variable CRTM_COEFFS_PATH available when the if (EXISTS ${FIX_FILES_PATH}) code path is taken above (instead of the download code path) ?
|
|
||
|
|
||
| set (CRTM_TESTFILES_PATH ${CRTM_COEFFS_PATH}/${CRTM_COEFFS_BRANCH}/fix) | ||
| message(STATUS "Set the file path to be seen by UFO at ${CRTM_TESTFILES_PATH} ") |
There was a problem hiding this comment.
@rajichidamb change this message to be model agnostic -- specifically, CRTM is primarily a stand-alone package/library, that UFO happens to use. So mentioning UFO here would be quite weird to most users.
4 participants
Description
CRTM coefficient is downloaded twice when building using jedi-bundle, once by crtm module and then by ufo and saved in two different locations. Changing the download location to be in jedi-bundle and then used as a symlink in the build directory for testing, reduces the download of coefficient to only once.
Issue(s) addressed
Resolves #https://github.com/JCSDA-internal/ufo/issues/4085
Dependencies
List the other PRs that this PR is dependent on:
build-group=https://github.com/JCSDA-internal/ufo/pull/4123
Impact
Expected impact on downstream repositories:
Checklist