Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Output of 'CO2EFFECTS' does not match output function #78

Closed
dschlaep opened this issue Nov 18, 2017 · 3 comments
Closed

Output of 'CO2EFFECTS' does not match output function #78

dschlaep opened this issue Nov 18, 2017 · 3 comments

Comments

@dschlaep
Copy link
Member

Values in output slot "CO2EFFECTS" are not calculated with the aggregation function that is specified.

This causes unit tests to fail:

rSOILWAT2 annual aggregation: ..........................1..........................2..........................3
Failed --------------------------------------------------------------
1. Failure: Simulate and aggregate (@test_exec_and_aggregate.R#48) --
{
    ...
} not equivalent to `res_true`.
128 element mismatches
test-data Ex1 - slot CO2EFFECTS
...

The output itself looks reasonable, so I don't think we need to change anything there.

@dschlaep dschlaep added the bug label Nov 18, 2017
@dschlaep dschlaep added this to the Rsoilwat_co2_effects milestone Nov 18, 2017
dschlaep added a commit that referenced this issue Nov 18, 2017
- all unit tests pass except 3 in scope "rSOILWAT2 annual aggregation"
    --> close #42, close #74, close #75, close #76, close #77
- the unit test errors are because the values of the output slot
"CO2EFFECTS" are not calculated with aggregation function that is
specified --> see new issue #78
@dschlaep
Copy link
Member Author

The culprit is the function get_co2effects: instead of doing what all other functions for the same functionality are doing, i.e., copy aggregated values to the output container, this function also does the aggregating itself -- and miserably fails. For instance, the code does not consider that duration of months differ. This means that the average across monthly values is not the same as the yearly average (across daily values) which these functions are supposed to calculate.

@Zachary-Kramer
Copy link
Contributor

So, yes, get_co2effects doesn't follow the traditional aggregation process, and it will need to be changed.

How would you approach this problem? The current solution is the way it is partly because I didn't see a straightforward way to integrate CO2 effects into the existing scheme, so your advice would be appreciated. I wish I could ask specific questions, but I don't really know where to start. So, I will just mention what I've observed and please correct me as I do so.

From my understanding, SW_SoilWater.h needs variables for carbon so that the averages can be automatically calculated. Currently, there are many outputs for carbon. There are 4 biomass variables and 4 biolive variables that need to be tracked. So, I should create the following variables: biomass_grass, biomass_shrub, biomass_tree, biomass_forb, biolive_grass, biolive_shrub, biolive_tree, biolive_forb in both SW_SOILWAT and SW_SOILWAT_OUTPUTS.

What I don't completely understand is how the sums and averages are being generated. For instance, how are values given to the variables in SW_SOILWAT_OUTPUTS? For instance, the CO2_biomass values exist in SW_VEGPROD.[PFT]. How can I link these biomass variables to SW_SOILWAT_OUTPUTS?

I also see that there is a function average_for in SW_Output.c. It seems that I have to add something like

case eSW_Carbon:
  savg->biomass_grass = ssumof->biomass_grass / div;
...

And then I will be able to access the averages. For CO2 output, do I have to do something similar in any other functions? e.g. sumof_swc

To give you an example of why it is hard for me to understand, I initially did look at similar outputs to try to develop CO2 in the same way. For instance, get_vwcBulk also uses SW_SOILWAT. I see that it is using a variable dysum.vwcBulk and try to find how it's being used so that I can do the same for carbon. So, I find all references to dysum.vwcBulk, but it is never assigned a value, which is odd, and neither it or dysum have documentation. I expand the search to just "vwcBulk" and still do not see where the average could be done. After investigating for a bit, it seems that the assignment was in fact in that huge search and is done with the variable name s->vwcBulk[i] += v->swcBulk[Today][i], and that is eventually used in the sums and averages. I still don't see where SW_SOILWAT gets its values from structs like SW_VEGPROD. So, even aspects that are simple in theory like centralizing the averaging can be hard to understand to developers that are new to that section of code, due to poor documentation and variables that are hard to find references to.

So, I hope that you could kindly explain a bit about the approach to aggregating in SOILWAT2. I would be happy to formally document your explanation within SW_Output/SW_Soilwater/SW_Weather.

@Zachary-Kramer
Copy link
Contributor

I've investigated this much more and think I understand the concept now. What was confusing beforehand was that I was trying to add biomass/biolive variables to SW_SOILWAT_OUTPUTS, but it does not fit there, as they are products of VegProd. So, I added a new output structure and functionality: SW_VEGPROD_OUTPUTS. From this point, I began understanding how everything is linked together. Right now there are some averaging issues, but I think it will be fixable on my own.

@dschlaep dschlaep self-assigned this Nov 30, 2017
dschlaep added a commit to DrylandEcology/SOILWAT2 that referenced this issue Nov 30, 2017
- close DrylandEcology/rSOILWAT2#78
- close DrylandEcology/rSOILWAT2#73

- file 'testing/Input/outsetup_v31.in'
* fully explain what output columns are
* rename output file to 'vegetation' from 'co2_effects'

- function '_collect_values()' did not call `SW_OUT_sum_today(eVPD)`

- return value of function 'get_co2effects' was 'void' instead of
'static void' as every other 'get_*' function

- function 'SW_OUT_flush' did not call `SW_OUT_sum_today(eVPD)`

- function 'sumof_vpd' only ever added biomass values into output slots
from 'Today' which is 1 -- the correct index is 'SW_Model.doy'

- function 'average_for' precluded 'ObjType's other than 'eSWC' and
'eWTH' to perform averaging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants