CABLE Library for AM3/ACCESS3 - #724
Conversation
|
This now, along with JULES PR #346, access-spack-packages PR #460 and UM PR #148 successfully builds AM3 with CABLE as a library and achieves bitwise compatible results. |
|
@Whyborn Can you please add the benchcab results for bitwise reproducibility? |
ccarouge
left a comment
There was a problem hiding this comment.
A few requests for more explanation and some relatively minor changes.
| spack-manifest-data-path: .github/build-ci/data/standard.json | ||
| # We need to install the compilers manually here because the upstream image is too large to run on GitHub-hosted runners | ||
| spack-compiler-manifest-path: .github/build-ci/compilers/spack.yaml | ||
| access-spack-packages-ref: 1d9acf45c39229ff4e90a9aa49922b1169305476 |
There was a problem hiding this comment.
What is this line doing? Why is it needed now and not before?
There was a problem hiding this comment.
That's to make the CI use the correct version of the spack packages. I should get the spack packages PR merged in first, tag it, then refer to that tag here.
There was a problem hiding this comment.
Oh actually I think I can remove that line completely when the spack packages update is merged, as I think it uses the head of the default branch if not specified?
There was a problem hiding this comment.
Why is this file modified for the library work? I would guess we do not compile this for AM3.
There was a problem hiding this comment.
This was intended to be a bit of future proofing, as Sean's MPI slightly modified the land use change code which was referred to by the AM3 code, even if not yet active. We came up with a better solution in this merged PR. I'll remove this change.
There was a problem hiding this comment.
Why are we adding this to science/ when it is only to be used in ESM1.5/1.6? Should the # define ESM15 YES be # define ESM15 NO in the offline repository? Or even, remove that line from the file and deal with it at compilation as usually done for pre-processor flags.
There was a problem hiding this comment.
And why is it part of the AM3 library work?
There was a problem hiding this comment.
Unfortunately the land use code had moved in a bit from the form that is in AM3. The JULES src/science_cable/landuse/landuse3.F90 should be the same as src/science/landuse/landuse3, but it isn't, and the coupling code is written based on the JULES version. A few routine signatures have changed, and I was getting type mismatch errors when I tried to quickly fix it.
I should remove the #ifdef and #def. I think it's best to get this in first, then reconcile the differences between the way this routine is called in the coupling and then remove this file.
There was a problem hiding this comment.
Files that are modified from the offline version for the coupled versions should all be stored under src/coupled/AM3. Can we put this under src/coupled/AM3/cable/landuse? If there are other files that are slightly different
Can you also clean up the src/coupled/AM3 directory tree of everything that isn't needed for the library, please? These files are not used and should not be kept around. As part of this PR, please.
| src/science/casa-cnp/casa_dimension.F90 | ||
| src/science/casa-cnp/casa_feedback.F90 | ||
| src/science/casa-cnp/casa_inout.F90 | ||
| src/science/casa-cnp/casa_param.F90 | ||
| src/science/casa-cnp/casa_phenology.F90 | ||
| src/science/casa-cnp/casa_readbiome.F90 | ||
| src/science/casa-cnp/casa_rplant.F90 | ||
| src/science/casa-cnp/casa_sumcflux.F90 | ||
| src/science/casa-cnp/casa_variable.F90 |
There was a problem hiding this comment.
What happens to all these CASA files for the ESM1.6 library? Were they added but are in fact not needed?
There was a problem hiding this comment.
Unfortunately these are bits that had been rewritten for AM3- the way the derived types are structured is different to offline, see this JULES issue. This is related to the duplicate ALLOCATABLE/POINTER type definitions that I wanted to unify some time back. The AM3 versions of these live in the JULES src/util/casa.
There was a problem hiding this comment.
I don't think I follow what you are saying. So these files are not the same between ESM1.6 and AM3, ok. But it does not explain what happens for these files for ESM1.6. Right now, these files completely disappear of the compilation of ESM1.6 library in this PR, making me doubt that the compilation of the ESM1.6 library would work..
At the same time, I'm pretty sure we said this version of CABLE won't be compatible with ESM1.6, so should we not include an ESM1.6 library in the CMakeLists.txt file?
There was a problem hiding this comment.
Yea, I think its either:
- Include those files in the ESM1.6 library build
- Drop the ESM1.6 library build altogether
With the conversations around BLAZE ongoing, I think it would be wise to keep the ESM1.6 library code there and add the files. I don't think it would be much work to make the ESM1.6 library build again, but keeping the list of files up to date would make future life easier if someone else was to come work on it.
There was a problem hiding this comment.
OK, when you have time, can you look at making sure the ESM1.6 library works. Can it also be added to the compilation CI in CABLE's PRs then so we would do 2 library builds?
There was a problem hiding this comment.
Can I make that a separate PR? Separate that work from the AM3 work.
There was a problem hiding this comment.
Should we keep the files in the list then so that it's easier to identify these files are problematic for the work on ESM1.6? Or comment them out with enough explanation to know what's needed?
There was a problem hiding this comment.
We can't leave them in the list uncommented, as the build with fail, but I think commenting them out with an explanation is a good idea.
|
Benchcab output: |
ccarouge
left a comment
There was a problem hiding this comment.
A few more things to do I think. Small things.
| src/coupled/AM3/control/cable/CM3/air_type_cbl.F90 | ||
| src/coupled/AM3/control/cable/CM3/balances_type_cbl.F90 | ||
| src/coupled/AM3/control/cable/CM3/bgc_pool_type.F90 | ||
| src/coupled/AM3/control/cable/CM3/cable_define_types.F90 | ||
| src/coupled/AM3/control/cable/CM3/cbl_canopy_type.F90 | ||
| src/coupled/AM3/control/cable/CM3/cbl_cbm_mod.F90 | ||
| src/coupled/AM3/control/cable/CM3/climate_type.F90 | ||
| src/coupled/AM3/control/cable/CM3/met_type_cbl.F90 | ||
| src/coupled/AM3/control/cable/CM3/radiation_type.F90 | ||
| src/coupled/AM3/control/cable/CM3/roughness_type.F90 | ||
| src/coupled/AM3/control/cable/CM3/soil_type_cbl.F90 | ||
| src/coupled/AM3/control/cable/CM3/ssnow_type.F90 | ||
| src/coupled/AM3/control/cable/CM3/sum_flux_type.F90 | ||
| src/coupled/AM3/control/cable/CM3/veg_type.F90 | ||
| src/coupled/AM3/control/casa/types/casa_balance_type.F90 | ||
| src/coupled/AM3/control/casa/types/casa_biome_type.F90 | ||
| src/coupled/AM3/control/casa/types/casa_files_type.F90 | ||
| src/coupled/AM3/control/casa/types/casa_flux_type.F90 | ||
| src/coupled/AM3/control/casa/types/casa_met_type.F90 | ||
| src/coupled/AM3/control/casa/types/casa_phenology_type.F90 | ||
| src/coupled/AM3/control/casa/types/casa_pool_type.F90 | ||
| src/coupled/AM3/control/casa/types/sum_casa_flux_type.F90 | ||
| src/coupled/AM3/control/casa/types/sum_casa_pool_type.F90 | ||
| src/coupled/AM3/control/casa/types/casa_variable.F90 | ||
| src/coupled/AM3/control/casa/types/phen_variable.F90 | ||
| src/coupled/AM3/control/casa/types/casa_dimension.F90 | ||
| src/coupled/AM3/control/casa/types/casa_inout.F90 | ||
| src/coupled/AM3/control/casa/types/casa_readbiome.F90 |
There was a problem hiding this comment.
I would simplify these paths:
src/coupled/AM3/cable/types/ and src/coupled/AM3/casa/types.
There was a problem hiding this comment.
Files that are modified from the offline version for the coupled versions should all be stored under src/coupled/AM3. Can we put this under src/coupled/AM3/cable/landuse? If there are other files that are slightly different
Can you also clean up the src/coupled/AM3 directory tree of everything that isn't needed for the library, please? These files are not used and should not be kept around. As part of this PR, please.
| src/science/casa-cnp/casa_dimension.F90 | ||
| src/science/casa-cnp/casa_feedback.F90 | ||
| src/science/casa-cnp/casa_inout.F90 | ||
| src/science/casa-cnp/casa_param.F90 | ||
| src/science/casa-cnp/casa_phenology.F90 | ||
| src/science/casa-cnp/casa_readbiome.F90 | ||
| src/science/casa-cnp/casa_rplant.F90 | ||
| src/science/casa-cnp/casa_sumcflux.F90 | ||
| src/science/casa-cnp/casa_variable.F90 |
There was a problem hiding this comment.
Should we keep the files in the list then so that it's easier to identify these files are problematic for the work on ESM1.6? Or comment them out with enough explanation to know what's needed?
|
Ok, I think this is ready to go. Aside from the main purpose of making the library for AM3, this PR:
Future work related to this is the unifying of the types used between the coupled and offline models. This should be possible with a "medium" amount of work. |
CABLE
Thank you for submitting a pull request to the CABLE Project.
Description
This builds the library for AM3 and future ACCESS3 applications. It requires a rejig of the original library, taking some files out of the default library and including them only in ESM1.6. The intention was that the library would only be the science code, but this is not possible with the current setup.
A summary of things that the science directory depends on, that extends outside the
sciencedirectory that I've identified:sciencedirectory. Each of these have different definitions for each application (offline, ESM1.6, AM3). Logically, there should only be one definition, but I don't think this will happen any time soon. In the mean time, including the type modules insrc/coupled/AM3/control/cable/CM3in the library should be an acceptable solution.cable_surface_types_modmodule also reads the namelist that sets the CABLE surface types. Do the surface types need to be handled in this way? Is there any foreseeable situation where anyone would not use the standard CABLE PFT indexing? If we remove this, it makes things significantly easier. It also does the extra work of defining a bunch of required JULES variables. I think the best solution is go back to hard-coded CABLE PFT indices, see this JULES PR.Testing
📚 Documentation preview 📚: https://cable--724.org.readthedocs.build/en/724/