Refactor of the CABLE-POP Met input routines. #290
Conversation
…ing. These changes permit the use of input datasets spanning multiple files by specifying a template which matches all the files in the dataset. The files in the dataset must contain YYYYMMDD specifications for the start and end date of the data contained in the specific file. The entry in the namelist is the template of the dataset, with <startdate> and <enddate> replacing the start and end date in the filename.
There was a problem hiding this comment.
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?
|
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. |
There was a problem hiding this comment.
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.
|
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 |
|
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. |
clarified. The documentation in the series of changes introduced to handle more generic met forcings now meets the documentation standards specified for CABLE. There were also a series of instances where the comments were not clear or contained mistakes.
The handling of nextTmin and prevTmax were incorrect at the start and end of the era of the datasets. Also corrected documentation in find_variable_ID.
|
The routines now achieve bitwise equivalence for the S1, S2 and S3 experiments. The various other subexperiments that existed in the |
Co-authored-by: Claire Carouge <[email protected]>
|
@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? |
|
@Whyborn can you resolve the "conversations" where changes have been made? This will allow me to know where we stand here. |
|
Conversations marked as resolved @ccarouge |
This reverts commit 9813bc0.
|
Results using the CABLE-POP_TRENDY branch have been bit-wise reproduced for the experiments |
|
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
|
|
My intention was to merge in my changes to the configuration at
|
The forcing files in Edit: They could be as silly as, not recommended obviously: @Whyborn I think the description of |
|
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 |
There was a problem hiding this comment.
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.
|
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. |
|
Compilation worked with gfortran. You could also try that on Gadi. |
There was a problem hiding this comment.
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.
|
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). |
|
I had a meeting canceled so pursued the topic. I was a bit too enthusiastic removing some unneeded parentheses in 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. Next time I will do an intermediate branch even for such "inconspicuous" as removing the temporary array creation, I promise ;-) |
|
Thanks @mcuntz for digging into this. It's good to know where it is coming from. |
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.F90module, to remove all hard-coded references to files and remove all internal switches hidden through the use of theRunnamelist 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
Checks
extent=64.0,66.0,60.0,62.0inrun_TRENDY.sh.