update logic surrounding timestepping of cable%cansto / %oldcanst

[JhanSrbinovsky]

CABLE

Thank you for submitting a pull request to the CABLE Project.

Description

Clarifying time stepping of cansto / oldcansto

Fixes #505

Type of change

Please delete options that are not relevant.

• Bug fix
• New or updated documentation

---

📚 Documentation preview 📚: https://cable--506.org.readthedocs.build/en/506/

[har917]

@JhanSrbinovsky @ccarouge

Headline comment: This looks okay - the science looks correct and it would certainly improve the consistency in logic between the different applications. It's a sensible half-way stop as part of the broader effort to address #162 #496.

However - one big concerns to check the approach on and one request for change.

concern: I'm not sure that I like edits to files in the coupled/CM2 and coupled/ESM1.5 directories - I would have thought these are now read only. However, you can't make the edit to cable_serial and cable_canopy and keep the codebase functional without those changes. So I'm at a bit of an impass on the way forward.

request for change: we need some comments in the code to describe what's going on better

---

In CM2/cable_explicit_driver, AM3/cable_explicit_driver and ESM1.5/cable_explicit_driver ahead of the new %cansto line add

!cbm/define_canopy() sets the value of %oldcansto then updates the value of %cansto
!we reset to value of %cansto to that at start of timestep here to work with the split structure of the ACCESS calls to CABLE

In cable_canopy ahead of new %cansto line include

!take a copy of the value of %cansto at the start of time step. This is used to reset %cansto part way through ACCESS timestep
!and in evaluation of some water fluxes.

[JhanSrbinovsky]

@har917 - hrmmm - you're right, but I have no idea of the way forward. Perhaps, and it will have to happen at some stage, MIPX models of tester-year will just need to be tagged and left alone.

[ccarouge]

concern: I'm not sure that I like edits to files in the coupled/CM2 and coupled/ESM1.5 directories
This is because #474 was merged into src/coupled/ESM1.5 instead of src/coupled/esm16. I've opened issue #507 and asked Ben to sort this out. Changes for ESM1.6 should go into src/coupled/esm16 (which hopefully can be renamed for consistency).

And I have no idea why CM2 need to change? CM2 does not run with CABLE3 so it should not reflect changes for the latest CABLE.

[har917]

@JhanSrbinovsky @ccarouge Another option would be to add conditions around frozen_limit, %wbice, %wband%ssat` that reflect the updated use of the different densities in the initialisation section(s) - i.e. constrain the starting conditions to abide by the updated physics. The key difference is that we wouldn't have to maintain conservation at that point.

This would, hopefully, avoid the initial temperature shock that you're seeing (which I still think is coming the restart not being in balance with the physics and showing up in surfbv). Given that we're breaking bitwise comparability anyway I don't think it's too much of an issue.

The question is where would it need to go in, presumably

• offline: in between the restart read and first time step
• coupled: in the init_soil part of the initialisation routines

Head's up: the work that @rkutteh has been doing (both the work to ensure consistency between soil type and veg type, and the GW work) could well intersect with this awkwardly.

[JhanSrbinovsky]

@ccarouge - true (I think). I have run CM2 semi-recently but I am not sure of the details. An argument though for just removing the coupled/CM2 directory completely?

@har917 - I'll have to double check but I don't remember having a restart for this GSWP2 run, although GSWP2 was the new kid on the block when I set this up so I might've just forgotten.

[ccarouge]

@JhanSrbinovsky We need some test results with this. What's the impact on simulations?

[JhanSrbinovsky]

Ive run this through benchcab. Surprisingly reproducibility fails. I can believe a rounding error perhaps, so I will have to plot the data, but my understanding is that it was just nodified so we could use the same time-stepping logic across apps, although the original time-stepping was not wrong per se.

[har917]

I'm a bit surprised this has failed on reproducibility given that I thought that we default to using cansat = 0.0. In that case this whole section of code should be effectively redundant (lots of 0.0 = 0.0 lines but no net impact).

However my bigger concern here is that there are changes to the ESM1.5 and CM2 *driver* files in this change set - I thought/hoped that we'd agreed to not modify the source code of past applications in the git repos and focus only on AM3 and ESM1.6.

More generally this area of science needs to be reviewed as part of cansto time stepping and the refactoring of the code base.

[JhanSrbinovsky]

hmmmm - I thought this was the conclusion if the review. Anyway, the coupled changes herein are likely in those apps already. They possibly appeared here before the decision had been made. I'll make sure that the coupled changes are indeed already in those apps and removed them. That shows up how crazy this is that it is not reproducible. We've moved the single line from outside canopy to inside canopy

[har917]

I'm a bit surprised this has failed on reproducibility given that I thought that we default to using cansat = 0.0. In that case this whole section of code should be effectively redundant (lots of 0.0 = 0.0 lines but no net impact).

Correction - it appears that (in offline and ESM1.5 at least) we are using cansat = veg%canst1 * veg*vlaiw with veg%canst1 = 0.1 (irrespective of the surface type!!)