Closed jkikstra closed 4 years ago
There are some things here that will need to be fixed before "releasing" PAGE-2020, but they are only worth thinking about now if they make getting to publication easier (and I'm not sure about that). First, we need to avoid global variables. The current use of version_growtheffects
is asking for trouble.
An important question is when is it appropriate to have options like this as parameters within components vs. as separate components. That's subtle, but there are three considerations:
The only way to satisfy both of these is to find ways to split apart components, so that additional features are captured in optional components. That's not always possible though.
Thanks for your points @jrising . These are very valid points and was planned to be addressed with the cleaning up, before going further. I admit I merged this too early, which has it a bit too messy, but I would also like to eventually avoid using global variables.
I had hopes that there would be a have multiple model versions in similar files (with files only separated for either annual or non-annual), but this seems impossible (or difficult).
Right now, after having 'fooled around' for some time, I get the feeling that, as you suggest, the only natural/possible way to do this, would be to create many extra component.jl files and then adapt the connections in main_model.jl for the versions one would want to use, and then to see how the MC simulations need to be adapted.
Would you agree @jrising?
I don't have a general answer for you. It depends on whether there are clean division points to split out different components. In any case, I don't think you should go back and reorganize the code for the growth effects.
Based on our discussions, Annual, Variability, and AR have now been merged in. This should be enough to produce SCC numbers for these model versions. Combinations of the models will follow later.
Rebased on master and ran the tests_extension, which runs fine.
Running runtests.jl
does not pass all tests however.
I'm thus suggesting postponing the merging until the tests have been updated. @jrising could you have a look at updating these? And once updated, consider approving this PR?
Current fails (not including those that are commented out by default) are due to:
Being aware of issue #54 , this can be merged in now and the discounting fix can be incorporated after that.
When completed, this PR will update release v1.0 (julia version of PAGE-ICE) to a version of PAGE which features options to run PAGE-ICE (with converging growth rates), an annual version of PAGE, PAGE with temperature variability (two different implementations), and PAGE with growth effects (both annual and non-annual).
Steps for combining:
Additional steps after merging: