Feature/AquaCropv7.2 integration #1633
Conversation
Testing was done for the generic crop having a 365-day cycle for 4 restart times (1 Feb, 1 Aug, 1 Dec, 1 Jan next sim), as well as for the AquaCrop Maize file planted on March 22nd. Minor differences are supposably introduced by precision differences (saving restart variables as single precision while AC uses doubles) but have no impact on the final biomass, yield, Tact, Eact. Note that the restart is not working for - perennial crops - GDD crops (will be fixed later) - salinity - (may need a more elaborate list somewhere) No irrigation nor groundwater files were used as it has been decided that those options will be forced from LIS. Irrigation will be introduced in the next developments. Groundwater will not be introduced for the time being.
|
An update: (1) Adding a call to initializeGlobalStrings in ac72_setup clears the problem reported on Friday. The code now fails in ac72_setup.F90 when accessing an uninitialized local variable (NrRuns). Given the number of uninitialized variables I've encountered so far, my conclusion is the code needs more work from the developers, and that updates (and probably a rerun of the provided test case) should be performed before providing this to NASA again. I can provide the code changes I made on my end upon request. The compiler flags I used to detect these problems are: CFLAGS = -c -g -Wall -Wcast-qual -Wcheck -Wdeprecated -Wextra-tokens -Wformat -Wformat-security -Wmissing-declarations -Wmissing-prototypes -Wpointer-arith -Wremarks -Wreturn-type -Wshadow -Wsign-compare -Wstrict-prototypes -Wtrigraphs -Wuninitialized -Wunused-function -Wunused-parameter -Wunused-variable -Wwrite-strings -fcheck=conversions,stack,uninit -fp-stack-check -fp-trap=common -fp-trap-all=common -ftrapv -traceback -DIFC -DLINUX -DLIS_JULES Note that compiling without the -check uninit flag causes the program to assume all variables have valid values. If -O0, the compiler will often set values to zero regardless of whether it makes sense to do so. For -O1 and above, the values in uninitialized variables are totally random (whatever happened to be in that particular piece of memory when it was assigned to the variable). |
|
Thank you for taking the time to find the problem, @emkemp! It is really appreciated. We can take it back from here and review all the uninitialized variables. I'll update this PR and re-run the test case. I will let you know when it is done. |
Passes test case with strict check compilation
Fixed restart bug to make sure that the soil moisture is not reset to FC. Test case runs with strict checks. Last step is to compile and run with recent compiler
Working version on wICE (was not able to compile with strict checks but -O0 works)
|
Hi David and Eric, The code is now ready for you to be tested again. Main modifications include:
Since we are already doing research with the code, I took the opportunity to already include two bug fixes:
The output of the test case (TARGET_OUTPUT) only slightly changed. The largest relative difference is ~10^(-4) but I suspect this can be due to a difference in compilation/system. The testcase output has been updated on Zenodo (https://zenodo.org/records/14887278, version 2.1.0). Only the aquacropv72test_output.tar.gz was updated, the ldt and lis input files remain unchanged. The test case was generated using the following lis.configure Let me know if you need any extra input from my side, or if the code would crash again on the Discover nodes. Many thanks! |
|
Hello: I successfully ran both LDT and LIS, and found minimal output differences with the latest targets. I also ran LIS in debug mode and found no issues. I cannot create a LDT executable in debug mode (due to unrelated code -- the executable takes too much memory at link time). However, the compiler did report some unused variables. I will prepare updates to remove them and save a little memory. |
|
Hi @lbusschaert Can you please open up permissions in your GitHub repo so I can upload changes? @jvgeiger and I are doing a code review and there are some (mostly cosmetic) changes that we would like to make. But I can't push updates to your repo. |
|
Hi @emkemp, I've made you both collaborators to my repo. I think you should be able to push changes now. If not, please let me know. Let me know if I can help with anything else. Thanks |
|
Hi @lbusschaert Hmm, I'm still being blocked. [emkemp@discover31 /discover/nobackup/projects/usaf_lis/emkemp/pr/1633/LISF/ldt]$ git push Please make sure you have the correct access rights @jvgeiger Do I need to push this to my personal GitHub repo first, and then push from there? |
|
Hi @emkemp, You are both still marked as "pending invite", maybe you could try to accept it first?
|
|
Hi @lbusschaert That was it! And my push went through (removing unused local variables). I'll work on additional updates. |
Description
This PR introduces the implementation of a crop model (AquaCropv7.2) in LIS, along with the LDT parameter processing. The implementation was done within the KU Leuven team of Prof. Gabriëlle De Lannoy, mainly by Michel Bechtold and myself.
Research has been done with a preliminary version of AquaCropv7.0 in LIS:
The objective was now to develop a final working version in LIS.
I was visiting the NASA-LIS team in early 2024 and prepared this implementation in collaboration with Sujay Kumar and David Mocko. The implementation is now finalized and has been carefully tested and compared to AquaCrop reference output.
AquaCropv7.2 (ac72) is now part of the surfacemodels/land/. The implementation is documented in
LIS_AC_documentation.pdf
We acknowledge that this is a large PR and we greatly appreciate the time you spend on it. Do not hesitate to contact me in case of any questions ([email protected]).
Testcase
Test cases for LDT and LIS have been added under the ldt/testcases and lis/testcases folders. The data for the test cases are stored in a Zenodo repository organized in the requested format: https://zenodo.org/records/14035685