Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GEOSldas changes prompted by cleanup of catch_constants in GEOSgcm GC #517

Merged
merged 7 commits into from
Feb 1, 2022

Conversation

gmao-rreichle
Copy link
Contributor

@gmao-rreichle gmao-rreichle commented Jan 27, 2022

Matches GEOS-ESM/GEOSgcm_GridComp#525

Merge this PR once the above GEOSgcm GC PR has been merged.

Before merge, restore "develop" version of components.yaml on this feature branch.

Preliminary testing is in progress. UPDATE: Intel tests with standard optimization passed. Full set of tests to be conducted by @biljanaorescanin when PR is "ready for review".

@gmao-rreichle
Copy link
Contributor Author

@weiyuan-jiang:
In putting together this PR, I noticed that two constants (N_gt and N_snow) that originate in the GEOSgcm_GridComp file:
GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/GEOSland_GridComp/Shared/catch_constants.f90
are also hardwired in a GEOSldas file:
./src/Components/GEOSldas_GridComp/Shared/catch_types.F90

It looks like at some point we had a "use..." statement in catch_constant.f90 that is now commented out:


I tried uncommenting this statement but the GEOSldas build fails because for some reason catch_types.F90 is compiled before catch_constants.f90.

Also, there is another GEOSldas file (LDAS_ensdrv_mpi.F90) in the same dir that also uses the same two constants (and currently pulls them from catch_types.F90):

use catch_types, only: N_gt, N_snow, N_cat_progn, N_cat_diagS, N_cat_diagF

Can we force CMake to compile catch_constants.f90 before catch_types.F90? Or would we need to change more than the order in which the files are compiled? It would be good to avoid the duplicate definitions of these constants.

@gmao-rreichle gmao-rreichle marked this pull request as ready for review February 1, 2022 22:29
@gmao-rreichle gmao-rreichle requested review from a team as code owners February 1, 2022 22:29
@gmao-rreichle
Copy link
Contributor Author

@biljanaorescanin, @weiyuan-jiang:
Scott just merged the associated GEOSgcm_GridComp PR: GEOS-ESM/GEOSgcm_GridComp#525
This means that we need to merge the present GEOSldas PR to keep the GEOSldas tests working.
I had tested the branch initially but not since it was finalized.
There isn't enough time today to test the branch before the nightly LDAS tests kick off. We could (1) merge the branch and let the automated nightly tests do the verification. Or we could (2) let the automated nightly tests fail today, do a manual test, and merge the branch tomorrow.
Since I initiated the PR, I can't approve/merge. I'll leave it up to you and your availability to tackle this. Either way is fine with me.
cc: @mathomp4

@biljanaorescanin
Copy link
Contributor

I will merge it since as you said it would fail anyway. And there is not enough time to run tests now.

@biljanaorescanin biljanaorescanin merged commit 0204905 into develop Feb 1, 2022
@gmao-rreichle gmao-rreichle deleted the feature/rreichle/cleancatchconstants branch February 1, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants