Refactor of the CABLE-POP Met input routines.

[Whyborn]

CABLE

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

Description

This is a work in progress for the new Met forcing routines for the CABLE-POP_TRENDY branch. This branch will involve some fairly major changes to the cable_cru_trendy.F90 module, to remove all hard-coded references to files and remove all internal switches hidden through the use of the Run namelist variable.

The new input routines allow the easy handling of datasets spanning multiple files. It is currently tested on the Met inputs, and should be able to be ported to other inputs without too much work.

The initial purpose of this branch is to bring the code to a state where we can run with ACCESS-ESM1.5 Met forcings with Land Use Change and Tree Demography science included, addressing #212 .

Type of change

• New feature

Checks

• Checked against small-scale TRENDY configuration, with extent=64.0,66.0,60.0,62.0 in run_TRENDY.sh.
• Checked against global configuration.

[mcuntz]

I appreciate any generalisation for reading spatial data. I find the approach to call the OS not very portable, especially when people are using different shells. There should be a library in Fortran that deals with the OS such as a the glob module in Python.

This is a pull request so it means that it was tested. Did it gave the same results as the "normal" Trendy S3 run, for example?

[Whyborn]

I am working on the TRENDY_v12 configuration that would go alongside these changes, reflecting the new IO routines and the changes to the namelist, see this pull request. This should be done over the next few days, and I will test against the original "S1", "S2", "S3" experiments for bitwise equivalence.

[ccarouge]

This is going to be a long process. So I've decided to leave several reviews focussing on different aspects. This one is on the documentation. Plus a few things I had put before deciding to split the review in bits.

First major comment. In cable_cru_TRENDY.F90 can you restore the order of the subroutines that existed before the modifications? I think it might make reviewing changes easier. Add the new (not modified, completely new) subroutines to the end if any.

I haven't always checked the comments against the original comments, it's ok if the reply to some suggestions is: Was in original version, leaving as is.

For the new documentation in general, it would be good if you could follow the new guidelines. Don't worry about getting the formatting exactly right since the pull request isn't going to build the doc for you. We can correct what we need when it gets into the main branch.

[Whyborn]

A lot of the commentary is in place to help both myself and reviewers, and will be changed to meet documentation guidelines before being pushed to a production branch. I'll address all the documentation changes in a single commit, then address each of the functional changes separately and reference the commit in a reply to each of the comments.

Would you like me to move the routines in cable_input.F90 to cable_cru_TRENDY.F90 for the review process (and maybe for the final product)? I think it makes sense to have them in cable_input.F90, but it might be better to have all the changes in one place.

[ccarouge]

I don't know yet where the routines in cable_input.F90 should be. I haven't yet looked at the code logic in enough detail.

Suggestions can also be applied directly from github in bulk. So you could add all the ones you agree to in one commit on github. Then add the other doc changes in a commit worked on locally and then the functional changes.

[Whyborn]

The routines now achieve bitwise equivalence for the S1, S2 and S3 experiments. The various other subexperiments that existed in the cable_cru_trendy.F90 are not available directly, but can be achieved through setting of the cru.nml namelist. The branch of TRENDY_V12 at 5-Update-Configuration-for-Met-Refactor) is a version which points to the symlinked Met data, renamed to the desired format.

[ccarouge]

There are quite a few comments. The main one I think is about setting the nMet variable and using that to remove all the special cases for the diffuse fraction.

Happy to discuss anything that isn't clear

[ccarouge]

There are quite a few comments. The main one I think is about setting the nMet variable and using that to remove all the special cases for the diffuse fraction.

Happy to discuss anything that isn't clear

[ccarouge]

One last question to resolve.

[ccarouge]

@Whyborn What tests have you run since the last changes to the code have been made? Can we ensure we still get bitwise reproducibility of previous simulations?

[ccarouge]

@Whyborn can you resolve the "conversations" where changes have been made? This will allow me to know where we stand here.

[Whyborn]

Conversations marked as resolved @ccarouge

[Whyborn]

Results using the CABLE-POP_TRENDY branch have been bit-wise reproduced for the experiments S0-3 on reduced domains using the adapted namelists at TRENDY_V12 (edited with updated link).

[mcuntz]

Dear Lachlan @Whyborn ,

I wanted to test the new met routines on our machine before accepting the changes. For this I would need to take the changes from the 5-Update-Configuration-for-Met-Refactor branch of the TRENDY_v12 repository. This is a bit convoluted now with my and your parallel amendments. Could you help me with this, please?

1. I take the changes in cru.nml.
2. I add a met_names.nml.
3. I have to provide the forcing files with the new names. Do you already have a nice one-liner to rename all files in a directory to the filename? Perhaps even all files in a list of directories? (I can do the co2 and ndep by hand ;-)
4. Then there are a few other changes in run_cable.sh. Do I need the *Recycle variables? Do I need CO2Method = "Yearly" (same for ndep)?

[Whyborn]

My intention was to merge in my changes to the configuration at TRENDY_V12 after you had merged your changes. It should be relatively easy to do, since the changes are quite separate.

1. Will depend on where you symlink the met data to
2. met_names.nml should be able to be taken as is
3. I do have a python script which I used to symlink the CRU-JRA_1x1 data at symlink_files.py
4. The Recycle denotes whether the Met data should be recycled, for CO2Method and NDepMethod are "Yearly" to get it from the Met year and YYYY to get it from a particular year.

[ccarouge]

@mcuntz:

I have to provide the forcing files with the new names.

The forcing files in cru.nml can be named in any way you want as long as they follow a template (for each variable) with a start date (YYYYMMDD format) and an end date (YYYYMMDD format).

Edit: They could be as silly as, not recommended obviously:
my_favourite_name_for_rain_<startdate>_I_like_to_separate_dates_<enddate>.nc

@Whyborn I think the description of File in the README should be beefed up to explain what the fields accept.

[Whyborn]

I had a longer description in the bulk of the README on the Met file template, but I've moved it to its own heading and linked to that heading under the File list item so it's easier to find.

[mcuntz]

cable_cru_TRENDY.F90 does not compile with gfortran due to incompatibilities with the Fortran standard. I wrote the correct statements in the comments.

Otherwise I adapted the TRENDY_v12 repository and ran a TRENDY S3 run with success, having identical output and restart files.

[Whyborn]

Great to hear. I've made the adjustments to the code to adhere to the Fortran standards. Hopefully you can now compile with gfortran. If you can, I'll close the conversations on those points.

[mcuntz]

Compilation worked with gfortran. You could also try that on Gadi.

[mcuntz]

Compiled with gfortran.

[ccarouge]

This looks good to go.

I have done a test for a single site using this branch and the head of the CABLE-POP_TRENDY branch. We don't expect differences since the new subroutines are only called if using the cru met. forcing for the moment, assuming other changes on the CABLE-POP_TRENDY branch were neutral to single site simulations.

The single site tests show identical values for the met forcing data. It shows very small differences in output variables. I've traced back at least some of the differences to the changes to qsatfjh() to avoid temporary arrays. Considering the tiny size of the differences, the fact the changes should only affect the CRU met forcings and the evidence of some numerical noise from other changes, I think it's safe to say these changes are neutral for the single sites.

[mcuntz]

It intrigued me that the change to the temporary arrays should give differences.

So I went back to the CABLE-POP_TRENDY version with temporary arrays and compared it at the flux site FR-Hes against this 212_... branch. It did not give the same results.

I have to investigate further but I won't be able to do it until next Friday (26/07).

[mcuntz]

I had a meeting canceled so pursued the topic. I was a bit too enthusiastic removing some unneeded parentheses in qsatfjh so the order of calculation changed, which changed the bitwise comparison. It's back with the parentheses ;-)

It's all good now. Would be great of @SeanBryan51 could check that the "temporary array creation" is gone with the Intel compiler.

So now there are changes to earlier runs but this comes from the LUC changes of Jürgen, I think. I tried checking out different commits but the met-changes are now intermingled with the other changes and I am lost in the commits.
But I tested here: 1. the removing of creation of temporary arrays worked at single site runs (landmask with 1 gridcell set to 1), and 2. the 212 branch gave the same results for an TRENDY S3 run.

Next time I will do an intermediate branch even for such "inconspicuous" as removing the temporary array creation, I promise ;-)

[ccarouge]

Thanks @mcuntz for digging into this. It's good to know where it is coming from.