494 redux local ice fixes

[JhanSrbinovsky]

CABLE

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

Description

Please include a brief summary of the change and list the issues that are fixed.
Please also include relevant motivation and context.

You can link issues by using a supported keyword in the pull request's description or in a commit message:

Fixes #(issue)

Type of change

Please delete options that are not relevant.

• Bug fix
• New or updated documentation

Checklist

• The new content is accessible and located in the appropriate section
• I have checked that links are valid and point to the intended content
• I have checked my code/text and corrected any misspellings

Testing

• Are the changes bitwise-compatible with the main branch? If working on an optional feature, are the results bitwise-compatible when this feature is off? If yes, copy benchcab output showing successful completion of the bitwise compatibility tests or equivalent results below this line.

• Are the changes non bitwise-compatible with the main branch because of a bug fix or a feature being newly implemented or improved? If yes, add the link to the modelevaluation.org analysis versus the main branch or equivalent results below this line.

Please add a reviewer when ready for review.

---

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

[JhanSrbinovsky]

@ccarouge I have several issues with testing required (apart from having limited time):

• The issue here didnt even show up until global run (is this even possible in benchcab/me.org). Now that that is otherwise "resolved" probably single -site tests are ok
• Anything short of uploading from gadi is untenable. My failed attempt and request for help (https://forum.access-hive.org.au/t/benchcab-me-org/4056) is still hanging

[JhanSrbinovsky]

with end of #494 comments/modifiations

[har917]

how is this SUBROUTINE getting hold of frozen_limit and Cdensity_ice? via cable_common?

Another comments on this routine for completeness - line 38 should be redundant.

[JhanSrbinovsky]

USE cbl_ssnow_data_mod

ll 38 - really? It doesnt need to be zeroed?

[har917]

It does in coupled - but all of this is inside a IF NOT %cable_runtime_coupled) condition so this line will never be encountered.

I'm not actually that convinced we need the outer IF condition either as this routine is now only ever encountered via the cbl_serial and mpiworker routines.

[JhanSrbinovsky]

@har917 ok - so I've looked at the whole file and not just the line 38.

I agree can remove the outer loop. If we want to zero dgdtg on first step of each run we can add it to um_init_*., without the ktau=1 thing. it looks like a per timestep var anyway so this should be fine just to give it a finite value.

[JhanSrbinovsky]

@ccarouge - I gather this was the outstanding change request you were referring to. Just waiting on Ians response

[har917]

@JhanSrbinovsky @ccarouge My point here is/was really that cbl_soilsnow_init_special.F90 is only ever encountered via the serial driver and mpi_worker routines (and possibly even only on the first call) and so that implies that the some of logical inside the routine is irrelevant (because it's trapped by logic at a higher level).

As such:

• the IF condition at line 35 is irrelevant (you never have cable_user@cable_runtime_coupled = .TRUE. in this subroutine).
• the IF conditions at line 37 and 81 are irrelevant (we shouldn't be encountering the subroutine twice)
• the IF condition at line 38 is irrelevant - this is an offline-only subroutine and anyway %dgdtg is always set in define_canopy before first use.

It could/would be sensible to include an initialisation of %dgdtg (and %dfh_dtg, %dfe_dtg and %dfn_dtg) cbl_um_init_soilsnow but that's not strictly needed.

It would be sensible to sort this out - if nothing else because it removes things like the need for the SAVE condition on ktau (i.e. applying the coding standards to these refactored parts of the code).

... BUT all of this is really PR creep. Leaving these lines in won't do any damage (as far as I can tell).

[JhanSrbinovsky]

These have been here since ACCESS-1.3 days and we just keep adding switches. The canopy% fields (like Claire has been looking at int JAC version of climate% decs) are all zeroed at declaration,allocation as was required there. If not zeroing for the moment is not causing a problem then I suggest we wait until we pick this up and be sure to CALL the zeroing SUBR. AND remove this code. Note aswell the coupled_runtime flag has effectively been re-purposed and should at least be renamed to reflect that it does not indicate whether it is in a coupled run or not. I'll make an issue from here

[JhanSrbinovsky]

@har917 The only use of ktau is at the bottom, outside of the runtime_coupled loop so it gets executed for everyone, and in the UM - (potentially) on every restart. BUT init_spec is never called in the UM anyway.

• Does offline ever run in chunks?

IF no, we can just use ktau_gl then and get rid of ktau completely.

Also, it strictly should be using **ssnow%**xx=heat_cap_lower_limit(:,1)

[JhanSrbinovsky]

unless of course that is what is being passed but I dont remember editing the offline drivers recently

[har917]

the only use of ktau is at the bottom, outside of the runtime_coupled loop so it gets executed for everyone, and in the UM.

Not as far as I can tell: at the moment

• spec_init_soilsnow is called from cable_serial and mpi_worker before the first time step and outside the timestepping loop (i.e. the routine only gets called once)
• spec_init_soilsnow is not encountered in AM3

That leaves ESM1.6 - going from the ESM1.5 cable_um_init routine it appears that the intent is to call spec_init_soilsnow from within an IF (first_call) construct. If that's correct then we can remove all mentions of ktau from this routine (and probably the if coupled conditions as well).

In rereading I also note that %gamzz set at line 75 is different to that set at 81 - which seems odd.

[har917]

A couple of new things noted - now as new issues.

Please check i) how frozen_limit and Cdensity_ice are getting into spec_soilsnow_init and ii) why we need a REAL() in one of the SUBROUTINE argument calls - if they're okay then this PR is approved.

[JhanSrbinovsky]

So @ccarouge - what are we going to do about testing here?

[Whyborn]

@JhanSrbinovsky there was a minor mistake in the new gridinfo- apparently xarray's DataArray.where, which I used to apply the ice fixes, silently casts the DataArray to floating point type, which caused the reading of isoil to fail. I have updated the file at the same location as before.

[JhanSrbinovsky]

@JhanSrbinovsky there was a minor mistake in the new gridinfo- apparently xarray's DataArray.where, which I used to apply the ice fixes, silently casts the DataArray to floating point type, which caused the reading of isoil to fail. I have updated the file at the same location as before.

Odd - my run didnt fail? Anyway, how do we go about replacing the grid_info in CABLE repo OR are we even concerned with doing that?

[Whyborn]

That's very strange, my test run failed with floating invalid in NF90_GET_VAR (and apparently it casted all the floating point parameters to 64bit floats, rather than 32bit so it broke all of them as well). I haven't gone anywhere near the gridinfo in the CABLE repository, I don't think that's the place to store ancillaries personally.

[JhanSrbinovsky]

That's very strange, my test run failed with floating invalid in NF90_GET_VAR (and apparently it casted all the floating point parameters to 64bit floats, rather than 32bit so it broke all of them as well).

This is a worry. Were you running in debug mode?

I haven't gone anywhere near the gridinfo in the CABLE repository, I don't think that's the place to store ancillaries personally.

Neither do I - storing binaries in there is contrary to the whole point really.

[JhanSrbinovsky]

tested version4 against version3 (gridinfo files). Identical results for me.

[Whyborn]

Yes, this was running in debug mode. I have since corrected the problem and managed to get a short run, with only 6 grid points and a single year, going with the ACCESS-ESM1.5 gridinfo and meteorology. Now extending it to a longer global run.

[JhanSrbinovsky]

Yes, this was running in debug mode.
Makes total sense then, I'm not worried now :)

Now extending it to a longer global run.

above comment (vn3 vs vn4) was for global GSWP2, so should work for others.

[ccarouge]

@JhanSrbinovsky for testing:

• show the flux site runs are identical (excerpt from benchcab log file) to show it doesn't change what it shouldn't change.
• for the spatial runs, in the discussion I see these results, it would be good for @har917 to give a general acceptance of the results (the differences shown look plausible and the testing is enough for the change). @JhanSrbinovsky have you said anywhere what the simulations are? It doesn't look like you did. Updating the PR description to add some information on the simulations you have done would be best. And in general, having any test results within the description (that can be updated anytime) is preferred to having them in separate comments.

And finally, @har917 needs to review again because he is still noted as requesting changes (or @JhanSrbinovsky you haven't put in all requested changes...).

For upload to me.org, Ben has been working on it and should get you a reply soon. me.org analysis is not needed here as we can't yet run ILAMB from me.org.

[JhanSrbinovsky]

show the flux site runs are identical (excerpt from benchcab log file) to show it doesn't change what it shouldn't change.

I reran the single site testing. It isnt bitwise identical. I never really expected it to be except maybe where the ice fixes (including ice density where there is any frozen liquid in soil) dont apply. Looking at the headline subR, soilsnow_main there is a kind=r_2 which is enough to destroy bitwise compatabiliy AND then there is correction of a bug in which wbliq was only local/.This made me wonder how they were ever identical. Returning to the early days - they never were identical.

for the spatial runs, in the discussion I see #527 (comment), it
would be good for @har917 to give a general acceptance of the results (the differences shown look plausible and the testing is enough for the change).

@JhanSrbinovsky have you said anywhere what the simulations are? It doesn't look like you did.
Updating the PR description to add some information on the simulations you have done would be best.

I will look at updating this

[har917]

@JhanSrbinovsky @ccarouge On the question of impact - it's a bit hard to give a definitive answer (partly because this has been a moving target).

Referring to map above - I'm assuming this is the net impact on one of the temperatures (which one?) after one day. If so this is reasonable since

• we expect a 'large' impact to come from the forced melting and discharge of frozen soils on the first time step
• forced melting induces a cooling of the soil - so MAIN - NEW will be mainly positive.
• temperature increment is mainly in the 1-2K range (physically plausible) - only regions outside of this range are a on couple of Artic islands (possible confused with the changes to the soil/veg conditions) and in Alaska (likely vert saturated soils.

I'd like a longer run to see if the temperature differences settle over time and/or we get something more systematic emerging - but no cause for concern so far.