Reconcile CABLE in ESM1.6 and CABLE:main #583
Conversation
There was a problem hiding this comment.
A reconciled version of this file that was previously in different locations per app
There was a problem hiding this comment.
A reconciled version of this file that was previously in different locations per app
There was a problem hiding this comment.
Whilst ESM1.6 and offline share the same code here, AM3 does not. Hence there are still multiple versions. This can be simplified so that atleast this part is a lone module USEd by AM3 and the rest just additional stuff to satisfy UM IO
There was a problem hiding this comment.
Whilst ESM1.6 and offline share the same code here, AM3 does not. Hence there are still multiple versions. This can be simplified so that atleast this part is a lone module USEd by AM3 and the rest just additional stuff to satisfy UM IO
There was a problem hiding this comment.
include UM7:#98Aust PFTs
There was a problem hiding this comment.
include UM7:#98USE surface_types
There was a problem hiding this comment.
include UM7:#98USE surface_types
There was a problem hiding this comment.
include UM7:#98Aust PFTs, surface_types
There was a problem hiding this comment.
include UM7:#98Aust PFTs
There was a problem hiding this comment.
include UM7:#98Aust PFTs
JhanSrbinovsky
added
the
priority:high
|
@bschroeter - I have requested @har917 to review the corresponding UM7 repo side PR |
|
Ben is on parental leave for a while, so I'll take over reviewing this. I'll merge this with the library fix branch and check that it builds. Not sure if it's worth running this through benchcab- I'll ask for opinions from Claire and Sean |
|
There was only one issue, to do with precision conflicts. Seems that soil%ssat_vec and soil%swilt_vec are declared only as REAL, but are used in a function requiring REAL(r_2). It looks like similar issues have been hit here, since most of the other arguments are wrapped in a REAL(..., r_2) conversion, so for now we'll just do the same with these conflicts. I think I will merge this branch into the 581-fix-library-build branch and do the testing from there, since that is set up to test the code in the coupled model, which is the desired outcome. |
|
cool. It was a while back, I cant think of where off the top of my head, but I did test it in coupled mode as well. The next PR #586 was adding all those little things that broke reproducibility. The r_2 issue is not so important coupled as the UM is built with -i8, -r8 so all REALs are double precision anyway. Modern compilers are clever enough to check the memory space requirement. From memory CMIP5 circa compilers checked the name and of course r_2 is not the same as REAL, even if they effectively are the same. Led to all sorts of problems. Probably one reason we moved to a library back then. Which was later reversed - hopefully we wont have the same issue. I dont think so, CABLE has matured in the UM somewhat. And we wont have so many changes inside CABLE science code. The problem we will have, if we do, is that CABLE offline will have to be verified |
|
Yea, I assume that the |
|
Just default to using whatever offline is doing. I'm not sure we use *_vec fields online at all. Well the headline is tha these *_vec fields are Ground Water model fields and there is no GW scheme online (yet), but I seem to remember a *_vec field is used somewhere. |
|
@JhanSrbinovsky, I know this is an old comment but you discussed about the Also, since @Whyborn is looking at this in the library PR, should we close this PR? I would assume all the changes here should be in Lachlan's PR and will come in when we move on to the library. |
|
Yea, I think we should close this PR. I have incorporated these changes into my branch of CABLE working on the library. |
CABLE
Thank you for submitting a pull request to the CABLE Project.
Description
Testing
2025-05-07 13:27:19,434 - INFO - benchcab.benchcab.py:391 - 0 failed, 20 passed
CAVEAT - with 2 minor bug fixes reversed. Addressed in subsequent PRXX.
i.e. CABLE:main and ESM1.6 versions of CABLE are only very slightly out of sync. At least in terms of code that is common to both apps. So slight are the differences that merging the two are expected to deliver identical the results. Initially, this was not the case. Reading through the merge(s) again revealed a few changes that would cause non-identical results.
There were 2 instances in the code that were in error and fixed in the ESM1.6 code, that to PFTs that should be included in conditional statements but were not. The third was due to use of the special real kind r_2 in decoration of a local variable in cable_soil_snow_main(). Reversing these changes, the reconciled code changes here lead to identical behaviour.
The code changes in this PR that give identical results + the 3 code changes that were reversed (which are already part of the ESM1.6 main branch) give identical results when testing in the coupled model.
It is perhaps this juncture in main:branch (following pushing it to main) that should be tested within AM3 for bitwise compatibility before these 3 extra changes are implemented there as well
link to ESM1.6 reconciliation PR
📚 Documentation preview 📚: https://cable--583.org.readthedocs.build/en/583/