Merge gridded_code into master #375
Conversation
This ensures that the RNGs are seeded regardless of gridded or non-gridded mode.
See the issue for more details
In ST_grid.c: I added the CellType struct with a few variables that I know need to be plot-specific. This struct is never instantiated in the code, it is only there to discuss and augment. In ST_stats.h: created ST_stats.h and moved all struct definitions from ST_stats.c to the new file. This way, we can instantiate accumulators in other files like ST_grid.c. In ST_stats.c: Moved all struct definitions out of this file. This file now includes ST_stats.h and uses the struct definitions from it.
Issue #256. Moving the randseed calls to before RunGrid() was resulting in different behavior with the same seed. To remedy this I have moved calls to randseed back to their former location, and added separate randseed calls in ST_grid.c in the equivalent location right after Plot_Initialize().
I added a new struct to sxw.h that contains all of the local variables from sxw.c This way we can instantiate these variables in each grid cell when using gridded mode. I named this struct TempType for now until we agree on a new name. The implementation of this struct in sxw.c is called SXWTemp. I am commiting these changes so that I can move on to other issues, and will change the names once we come up with something descriptive. I also added two local variables to the transp_window struct: added_transp and lastYear. These were free floating variables that need to be cell specific.
transp_window, SXW, and SXWTemp now have equivalents in the CellType struct. Additionally I added some documentation to the struct.
TempType -> SXW_resourceType SXWTemp -> SXWResources Temp was used to denote temporary names. These new names are permanent, barring us coming up with a more descriptive name.
Globals, Plot, Env, and Succulent and now pointers. They are allocated in a new function called allocate_Globals() I also removed the function _globals_init from ST_params.c because it serves no purpose anymore. All memory allocation is handled in the new function.
(issue #272 and #273) allocate_gridCells allocates memory for the 2d gridCells array. load_cell moves the Global variables to point to gridCells[row][col]'s fields. I still need to figure out how to load the SXW variables and Stats variables since they aren't globals. Neither of these functions are implemented yet.
(issue #275) These variables are now pointers that are dynamically allocated at run time. This changes nothing in non-gridded mode, but will make gridded mode capable of shallow copying it's data.
This new function allows us to shallow copy accumulators even though accumulators aren't global variables. To achieve this the new function takes pointers to the location of the accumulators that we want to copy.
To accomplish this I created a new function in sxw.c called _allocate_memory which should be called once per iteration. This not only changes tranps_window to a pointer, but also the arrays inside on transp_window. This ensures that we can easily reassign memory.
Not really a gridded code issue but it has been annoying me.
This function will deallocate any dynamically allocated pointers in sxw.c.
Both variables are now allocated in _allocate_memory in sxw.c. In ST main there was one line in init_args that referenced SXW before we could possibly allocate the memory for SXW. To remedy this I moved the line to SXW_Init.
(issue #272) This takes care of shallow copying the accumulators.
changed to copy_sxw_variables to match sxw.c
ST_stats.c externed "SXW_t SXW", but never used the variable. I removed this line and nothing changed.
For now this variable has been changed to a pointer and allocated in allocate_Globals. This allows us to move forward with the branch but is not the most readable solution.
(issue #286) This is necessary to ensure that the grid cells are available as soon as possible.
I moved _init_grid_inputs in front of _init_stepwat_inputs so we can allocate the gridCells before they are needed.
In RunGrid: declared all local variables at the start of the function. I also moved the function description to the top of the function, since most IDEs look there.
(issue #293) This allows each grid cell to hold its own soil info. I also changed the name of the type to SoilType for readability. I added some comments for each variable to make them easier to understand. Lastly, I moved the description of the structs to the top since most IDEs look there for information.
(pull request #375)
In ST_grid.c: * Added references to GitHub in the file header. * Reduced the maximum column to 80 in the file header. This makes it displayable on most monitors and *might* encourage people to read it. * Corrected typos. * Added "_" prefixes for local functions that didn't have them already. * Updated some weak comments. * Fixed some dangling if statements. In ST_grid.h: * Removed "_" prefix from an exported struct. In ST_progressBar.c: * Fixed some typos. * Changed parameter names for the _calculateProgress function to make them more robust.
|
@kpalmqui I am now happy with the changes. Once you have a chance to review we can merge this branch and begin working on seed dispersal. |
|
@chaukap I should be able to get this done by Friday. |
Fixed a typo and improved the documentation of _calculateProgress.
There was a problem hiding this comment.
@chaukap my initial review. Not done - still have half the files to go through. Will try and finish within the next 24 hours.
I added a few questions to single lines that have been removed just to make sure they were removed correctly (I'm sure they were) - forgive me - it was too hard to go back through all of the commits and figure it out.
There was a problem hiding this comment.
I have gone through all files carefully. I embedded some questions regarding removed lines, I apologize if their removal is obvious somewhere in the commits, but I just wanted to be sure we didn't delete something accidentally. Mainly I have minor requests.
However, we have deleted the functions currently in place for seed dispersal. This worries me because some of the logic in the current code that comes directly from Coffin and Lauenroth 1989 (which is what we still want to implement) has been deleted. I would prefer that we retain the original functions (commented out), so we make sure we aren't missing anything when we overall seed dispersal in the next development branch.
Mostly spelling mistakes and a few comment improvements. The more concerning issues were not corrected with this commit.
* Removed calls to parm_Initialize in ST_grid.c and ST_initialization.c * Removed call to proportion_Recovery in spinup. * Modified some documentation.
Mostly documentation, and in ST_seedDispersal I added back a few lines of code that could help us when overhauling seed dispersal.
* Removed makeHeader option for seed dispersal output. The header is now made regardless. * Removed SXW_BYMAXSIZE in sxw.c. * Removed an old comment.
Some of the input CSVs had over 100 cells specified by default, which was a little excessive. I limited the number to 16, so the user can run a 4 x 4 grid without changing inputs.
This code snippet confused me for a long time. Without it the program crashed, but it was unclear why. I did some investigating and this is the best explaination I could come up with.
|
@kpalmqui I believe I have addresses your reviews entirely. If you want any more clarification on any of my comments please let me know or we can discuss them on our next call. The only remaining issue is seed dispersal. Have you decided whether or not we should retain the seed dispersal code for now? |
|
@chaukap great thank you! I will re-review. Let's retain seed dispersal and deal with it in the next branch as originally planned. |
There was a problem hiding this comment.
@chaukap I want to do a little more testing with the latest commits to confirm all is good.
Would you be able to test non-gridded mode within gridded_code and compare it to master branch a few times with different disturbance inputs and ensure all of the output is still the same?
|
@kpalmqui I just ran 6 single-plot simulations on both master and gridded code. Heres the parameters I used:
To compare the output I looked at the average values for temperature, precipitation, sagebrush biomass and a.cool.forb biomass. In all cases the values were Identical. To double check, in the simulations with fire I checked the years that fire occurred and they were also identical. I therefore conclude that single-plot mode has not been altered by the changes on this branch. |
The SOILWAT2 companion branch to gridded_code was just merged into master. Before I merge gridded_code into master I needed to update the SOILWAT2 head to incorporate the changes. After this commit (and testing) I will merge gridded_code.
|
@kpalmqui Thank you for all your guidance! I'm waiting for the Appveyor checks to clear then I'll get this branch folded in. The companion SOILWAT2 and rSFSTEP2 branches have been merged in already. |
I am opening this pull request to allow us to review the code as we test it. There are a ton of changes on this branch so it will take us some time to work through everything.
The biggest lingering issue is Spinup. Therefore, it would be best to start reviewing all files except ST_initialization.c. Once we have a concrete plan for Spinup we can finish the review.
Things to Note
-pflag. These files are be 100% complete and bug free (in theory).