DrylandEcology / SOILWAT2

An ecosystem water balance simulation model
GNU General Public License v3.0
6 stars 2 forks source link

Specified output variable/time period combinations are not being outputted as requested #120

Closed kpalmqui closed 6 years ago

kpalmqui commented 6 years ago

When I added additional daily output for several variables in the outsetup_v30.in of STEPWAT2, those columns were not outputted (for example TEMP in the sw2_daily.csv).

Also yearly PRECIP and AET is not being outputted in the sw2_yearly.csv file. Yearly SWA is not being generated in the sw2_yearly_slyrs.csv.

Monthly output for TRANSP and SWA is not being outputted to the monthly file.

I was running with just the -o option. I have attached the edited outsetup_v30.in file I was using.

outsetup_v30.txt

BrendenBe1 commented 6 years ago

The solution to this will be having steppe use the timestep value instead of adding another line for each variable. Currently in SW_Output.c around line 621 there is a line useTimestep = 0; and that makes it so only the timeperiod the values are defined for will be used instead of using the timestep line in the outsetup file to run those timeperiods on each variable. I need to resolve a memory problem before switching this though.

kpalmqui commented 6 years ago

I requested daily and yearly TEMP and PRECIP, and daily output for other variables (see attached outsetup_v30.in). I see no daily TEMP and PRECIP columns in the sw2_daily.csv files. outsetup_v30.missingdailyinfo.txt

sw2_daily.xlsx sw2_yearly.xlsx

What's going on here?

kpalmqui commented 6 years ago

After pulling this commit, I am now getting a completely blank (no headers) sw2_daily.csv and sw2_daily_slyrs.csv file when using the USE_TIMESTEP=0 option and the attached outsetup_v30.in

outsetup_v30.txt

kpalmqui commented 6 years ago

I will re-test given you new commit.

BrendenBe1 commented 6 years ago

I am trying to get the TIMESTEP=1 to work so test that with this latest commit. Dont repeat values with different periods, instead set timestep = 1; it should be working.

outsetup_v30.txt

kpalmqui commented 6 years ago

With USE_TIMESTEP=1, the daily temperature values look good. The daily Precip values in sw2_daily.csv remain problematic. They are zero initially and then appear to be yearly values repeated over and over for each day of year (an issue I believe we have previously documented, but I could not find it on Github). All of the other variables in the sw2_daily.csv file look fine. I will need a bit more time to make sure the variables in sw2_daily_slyrs look good, along with sw2_yearly_slyrs and sw2_yearly. And to confirm that all of the requested variables are being written to the files. I'll have feedback on all of this for you tomorrow.

Note that the reason I re-opened this issue is that not all of the requested variables were being written when USE_TIMESTEP=0. I am glad USE_TIMESTEP=1 seems to be working properly now (still need additional confirmation from me), but issues still remain with USE_TIMESTEP=0 I fear.

BrendenBe1 commented 6 years ago

What is the problem with USE_TIMESTEP = 0? Is it that not all variables are being written when multiple time periods are defined such as TEMP MO and TEMP YR? That functionality was intended for use before we implemented the USE_TIMESTEP flag. Now that we can do multiple timeperiods when USE_TIMESTEP = 1 that old functionality is no longer needed as I understood it.

kpalmqui commented 6 years ago

I thought the utility of USE_TIMESTEP=0 was allowing the user to request daily information for one variable and yearly information for another. (ie. daily Transpiration and yearly AET).

With USE_TIMESTEP=1, you would always get daily and yearly info (if requested) for ALL variables (i.e daily and yearly Transpiration and daily and yearly AET). Please correct me if this is not the case.

And yes, not all variables were being written when multiple time periods are requested using the USE_TIMESTEP=0. However, after your recent commits, the sw2_daily_slyrs.csv and sw2_daily.csv files are completely empty (no headers).

BrendenBe1 commented 6 years ago

yes, that is the intended functionality of USE_TIEMSTEP = 0. It allows for each variable to have a different timeperiod but not for a single variable to have multiple timeperiods. I cant figure out why this functionality was working before but I suspect that the values must have been wrong for those instance.

kpalmqui commented 6 years ago

I see. Would it be possible to change this so it does allow multiple time periods for each variable?

BrendenBe1 commented 6 years ago

I'm not sure. I will look into it today.

BrendenBe1 commented 6 years ago

The period that it does for each value is stored in a variable. When Timestep is used it sets the same period for each value as defined in the outsetup file on the TIMESTEP line. When timestep isnt used it sets it equal to the period defined. I dont think we can set it to just do an extra run for one value though.

kpalmqui commented 6 years ago

That would have been nice, but ok.

I have tested the USE_TIMESTEP=1 and USE_TIMESTEP=0 options to save SOILWAT2 output within STEPWAT2 and all of the output looks good, with the exception of a few variables, which I think are existing problems within SOILWAT2 standalone? @BrendenBe1 @dschlaep @CaitlinA

1) SOILTEMP, RUNOFF, and SURFACEWATER are always 0. 2) Daily PPT is either 0 or yearly values.

dschlaep commented 6 years ago

That would have been nice, but ok.

With PR #126, I fixed bugs in the SOILWAT2-standalone output code and eliminated inefficiencies (e.g., the same output values were calculated multiple times, but only outputted once).

From documentation of SW_OUT_read:

We have two options to specify time steps:

  • The same time step(s) for every output: Add a line with the tag TIMESTEP, e.g., TIMESTEP dy mo yr will generate daily, monthly, and yearly output for every output variable. If there is a line with this tag, then this will override information provided in the column PERIOD.
  • A different time step for each output: Specify the time step in the column PERIOD for each output variable. Note: only one time step per output variable can be specified. */

However, either way, the global variable

OutPeriod timeSteps[SW_OUTNKEYS][SW_OUTNPERIODS];// array to keep track of the periods that will be used for each output

is holding information in the first

int used_OUTNPERIODS; // number of different time steps/periods that are used/requested

columns about the output for each output key separately. There is no restriction for STEPWAT2 to not set-up different and separate output periods for different output keys, e.g., to request daily and monthly for the eSW_Transp key and yearly for the eSW_Precip key:

used_OUTNPERIODS = 2;
timeSteps[eSW_Transp][0] = eSW_Day;
timeSteps[eSW_Transp][1] = eSW_Month;
timeSteps[eSW_Precip][0] = eSW_Year;
timeSteps[eSW_Precip][1] = SW_MISSING;

I added the ability to set timeSteps to SW_MISSING for rSOILWAT2 for which it works; I have not tested it for SOILWAT2-standalone and it may need a bit more careful coding in (some of) the switch (timeSteps[k][i]).

SOILTEMP, RUNOFF, and SURFACEWATER are always 0.

Daily PPT is either 0 or yearly values.

This looks like a bug in the development branch. This is not the case on master:

git checkout master
make cleaner bint_test
nano testing/Output/precip.dy

gives me

1980    1   0.220000    0.000000    0.220000    0.000000    0.000000
1980    2   0.720000    0.000000    0.720000    0.000000    0.019606
1980    3   0.220000    0.000000    0.220000    0.000000    0.023012
1980    4   0.550000    0.000000    0.550000    0.000000    0.018803
1980    5   0.020000    0.000000    0.020000    0.000000    0.023113
1980    6   0.000000    0.000000    0.000000    0.000000    0.023469
...

with columns corresponding to

/ total precip = sum(rain, snow), rain, snow-fall, snowmelt, and snowloss (cm) /

BrendenBe1 commented 6 years ago

I am currently in the process of merging master into my development branch and most of these problems are being addressed. Hopefully I can push the changes today to my branch once I get everything working with the merge.

kpalmqui commented 6 years ago

Great. I would then like the opportunity to re-examine the output again in detail.

BrendenBe1 commented 6 years ago

I pushed the merge. There are a couple things I need to look more into and I will take care of that on Thursday but as of now the merge is complete and the majority of the functionality should be working.

kpalmqui commented 6 years ago

Daily PPT is either 0 or yearly values. This is now fixed in the soilwat_save_output branch

There is no restriction for STEPWAT2 to not set-up different and separate output periods for different output keys, e.g., to request daily and monthly for the eSW_Transp key and yearly for the eSW_Precip key: used_OUTNPERIODS = 2; timeSteps[eSW_Transp][0] = eSW_Day; timeSteps[eSW_Transp][1] = eSW_Month; timeSteps[eSW_Precip][0] = eSW_Year; timeSteps[eSW_Precip][1] = SW_MISSING; I added the ability to set timeSteps to SW_MISSING for rSOILWAT2 for which it works; I have not tested it for SOILWAT2-standalone and it may need a bit more careful coding in (some of) the switch (timeSteps[k][i]). Still not functional with STEPWAT2. Let's discuss what needs to be done in our call

BrendenBe1 commented 6 years ago

Replaced all uses of 'period' with timeSteps[][] and it is functioning. Still does not work with multiple timeperiods for a single value.

kpalmqui commented 6 years ago

We decided to put off this implementation for now. Ultimately, it would be great it be able to specify different timeSteps for different SOILWAT2 variables within STEPWAT2.

dschlaep commented 6 years ago

Closed with commits cff9a0511deaaad820dc235331ea5c432453ad05 and https://github.com/DrylandEcology/STEPWAT2/commits/49e43490d22ca265eaa4a5be73505bf7fa835ff4