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

Timestep and Calendar Modifications #188

Merged
merged 13 commits into from Jan 21, 2015

Conversation

Projects
None yet
3 participants
@jhamman
Member

jhamman commented Jan 12, 2015

Initial mods for subhourly timestep and alternative calendar support

  • Timestep units changed from hours to seconds,
  • Ported netcdftime.py to vic_time.c supporting all netCDF calendars.

Addresses #42 and #70 and contributes to #179.

To do (in upcoming commits):

  • Port to image driver
  • Test for all calendar and time units options
  • Test wide range of variable combinations in initialize_atmos.c
  • Code review from @bartnijssen
  • Code review from @tbohn
Joe Hamman
Initial mods for subhourly timesteps and alternative calendar support
- Timestep units changed from hours to seconds,
- Ported netcdftime.py to vic_time.c supporting all netCDF calendars.

Not stable.  Timestep/Calendar commit num. 1.
@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Jan 13, 2015

Member

Note: the Travis build failed only for driver/image which I have not made mods to yet.

Member

jhamman commented Jan 13, 2015

Note: the Travis build failed only for driver/image which I have not made mods to yet.

@bartnijssen

This comment has been minimized.

Show comment
Hide comment
@bartnijssen

bartnijssen Jan 13, 2015

Member

@jhamman
In
https://github.com/jhamman/VIC/blob/feature/subhourly_timestep/vic_run/include/vic_physical_constants.h

put parentheses around every mathematical operation that is done in a #define statement.

For example,

#define MIN_PER_DAY MIN_PER_HOUR * HOURS_PER_DAY

should read

#define MIN_PER_DAY (MIN_PER_HOUR * HOURS_PER_DAY)

Similarly:

#define MIN_PER_DAY (MIN_PER_HOUR * HOURS_PER_DAY)  /**< hours per day */
#define SEC_PER_HOUR (SEC_PER_MIN * MIN_PER_HOUR)  /**< seconds per hour */
#define SEC_PER_DAY (SEC_PER_HOUR * HOURS_PER_DAY)  /**< hours per day */
#define CONST_OMEGA (2.0 * CONST_PI / CONST_SDAY)
#define CONST_RGAS (CONST_AVOGAD * CONST_BOLTZ) /**< Universal gas constant ~ J/K/kmole */
#define CONST_RDAIR (CONST_RGAS / CONST_MWDAIR)  /**< Dry air gas constant ~ J/K/kg */
#define CONST_RWV (CONST_RGAS / CONST_MWWV)  /**< Water vapor gas constant ~ J/K/kg */
#define CONST_EPS (CONST_MWWV / CONST_MWAIR)  /**< Ratio of molecular weights */
#define CONST_TSTD (CONST_TKFRZ + 15.0)  /**< (K) standard temp at 0.0 m elevation */
#define CONST_RHODAIR (CONST_PSTD / (CONST_RDAIR * CONST_TKFRZ))  /**< density of dry air at STP ~ kg/m^3 */

We should similarly check the other header files.

Keep in mind that the precompiler does textual substitution for #define. So

day = (double)date->day + (double)date->dayseconds / (double)SEC_PER_DAY;

becomes

day = (double)date->day + (double)date->dayseconds / (double) SEC_PER_HOUR * HOURS_PER_DAY;

which becomes

day = (double)date->day + (double)date->dayseconds / (double) SEC_PER_MIN * MIN_PER_HOUR * HOURS_PER_DAY;

which becomes

day = (double)date->day + (double)date->dayseconds / (double) 60 * 60 * 24;

which becomes

day = (double)date->day + (double)date->dayseconds * 24;

which is a big number for any dayseconds > 0. You just have to be careful with the define statements.

Note that if you put the parentheses around the statements, you will have

day = (double)date->day + (double)date->dayseconds / (double) ((SEC_PER_MIN * MIN_PER_HOUR) * HOURS_PER_DAY);

which will give you the right answer.

Member

bartnijssen commented Jan 13, 2015

@jhamman
In
https://github.com/jhamman/VIC/blob/feature/subhourly_timestep/vic_run/include/vic_physical_constants.h

put parentheses around every mathematical operation that is done in a #define statement.

For example,

#define MIN_PER_DAY MIN_PER_HOUR * HOURS_PER_DAY

should read

#define MIN_PER_DAY (MIN_PER_HOUR * HOURS_PER_DAY)

Similarly:

#define MIN_PER_DAY (MIN_PER_HOUR * HOURS_PER_DAY)  /**< hours per day */
#define SEC_PER_HOUR (SEC_PER_MIN * MIN_PER_HOUR)  /**< seconds per hour */
#define SEC_PER_DAY (SEC_PER_HOUR * HOURS_PER_DAY)  /**< hours per day */
#define CONST_OMEGA (2.0 * CONST_PI / CONST_SDAY)
#define CONST_RGAS (CONST_AVOGAD * CONST_BOLTZ) /**< Universal gas constant ~ J/K/kmole */
#define CONST_RDAIR (CONST_RGAS / CONST_MWDAIR)  /**< Dry air gas constant ~ J/K/kg */
#define CONST_RWV (CONST_RGAS / CONST_MWWV)  /**< Water vapor gas constant ~ J/K/kg */
#define CONST_EPS (CONST_MWWV / CONST_MWAIR)  /**< Ratio of molecular weights */
#define CONST_TSTD (CONST_TKFRZ + 15.0)  /**< (K) standard temp at 0.0 m elevation */
#define CONST_RHODAIR (CONST_PSTD / (CONST_RDAIR * CONST_TKFRZ))  /**< density of dry air at STP ~ kg/m^3 */

We should similarly check the other header files.

Keep in mind that the precompiler does textual substitution for #define. So

day = (double)date->day + (double)date->dayseconds / (double)SEC_PER_DAY;

becomes

day = (double)date->day + (double)date->dayseconds / (double) SEC_PER_HOUR * HOURS_PER_DAY;

which becomes

day = (double)date->day + (double)date->dayseconds / (double) SEC_PER_MIN * MIN_PER_HOUR * HOURS_PER_DAY;

which becomes

day = (double)date->day + (double)date->dayseconds / (double) 60 * 60 * 24;

which becomes

day = (double)date->day + (double)date->dayseconds * 24;

which is a big number for any dayseconds > 0. You just have to be careful with the define statements.

Note that if you put the parentheses around the statements, you will have

day = (double)date->day + (double)date->dayseconds / (double) ((SEC_PER_MIN * MIN_PER_HOUR) * HOURS_PER_DAY);

which will give you the right answer.

@@ -470,7 +470,7 @@ display_current_settings(int mode)
fprintf(LOG_DEST, "\n");
fprintf(LOG_DEST, "Output Data:\n");
fprintf(LOG_DEST, "Result dir:\t\t%s\n", filenames.result_dir);
fprintf(LOG_DEST, "OUT_STEP\t\t%d\n", global_param.out_dt);
fprintf(LOG_DEST, "OUT_STEP\t\t%f\n", global_param.out_dt);

This comment has been minimized.

@bartnijssen

bartnijssen Jan 13, 2015

Member

check %f of %lf

@bartnijssen

bartnijssen Jan 13, 2015

Member

check %f of %lf

@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Jan 13, 2015

Member

I see. I see. That is much clearer now and I will implement that change shortly.

Member

jhamman commented Jan 13, 2015

I see. I see. That is much clearer now and I will implement that change shortly.

@tbohn

This comment has been minimized.

Show comment
Hide comment
@tbohn

tbohn Jan 13, 2015

Shouldn't sec be declared as an int? TmaxSec and TminSec are both arrays of ints, and the for loop beginning on line 131 increments sec by SEC_PER_DAY, presumably this is an integer amount (86400)? And the set_min_max_sec() function declares sec to be an int there...
UW-Hydro#188

Shouldn't sec be declared as an int? TmaxSec and TminSec are both arrays of ints, and the for loop beginning on line 131 increments sec by SEC_PER_DAY, presumably this is an integer amount (86400)? And the set_min_max_sec() function declares sec to be an int there...
UW-Hydro#188

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Jan 14, 2015

Owner

I think it actually makes more sense to change the type of TmaxSec and TminSec to double since that is how we are viewing the steps size.

Owner

jhamman replied Jan 14, 2015

I think it actually makes more sense to change the type of TmaxSec and TminSec to double since that is how we are viewing the steps size.

This comment has been minimized.

Show comment
Hide comment
@tbohn

tbohn Jan 14, 2015

tbohn replied Jan 14, 2015

@tbohn

This comment has been minimized.

Show comment
Hide comment
@tbohn

tbohn Jan 13, 2015

Should step be replaced here by step * stepsize, so that the units (seconds) are consistent with i * SEC_PER_DAY? Actually, looking below at lines 203-204 and 211-212, I see different indexing again; this time "i" is multiplied by stepsperday instead of SEC_PER_DAY, which actually makes more sense. So, perhaps "step" should be left as it is, and instead SEC_PER_DAY should be replaced by stepsperday? But then, in 211-212, "sec" would need to be replaced by "step".
UW-Hydro#188

Should step be replaced here by step * stepsize, so that the units (seconds) are consistent with i * SEC_PER_DAY? Actually, looking below at lines 203-204 and 211-212, I see different indexing again; this time "i" is multiplied by stepsperday instead of SEC_PER_DAY, which actually makes more sense. So, perhaps "step" should be left as it is, and instead SEC_PER_DAY should be replaced by stepsperday? But then, in 211-212, "sec" would need to be replaced by "step".
UW-Hydro#188

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Jan 14, 2015

Owner

Correct. The index in this line should have read i * stepsperday + step. In the lines below, I have replaced the index sec with step.

Owner

jhamman replied Jan 14, 2015

Correct. The index in this line should have read i * stepsperday + step. In the lines below, I have replaced the index sec with step.

This comment has been minimized.

Show comment
Hide comment
@tbohn

tbohn Jan 14, 2015

tbohn replied Jan 14, 2015

@tbohn

This comment has been minimized.

Show comment
Hide comment
@tbohn

tbohn Jan 13, 2015

Again, why is sec declared as double here, when tmaxsec and tminsec are declared as ints?
UW-Hydro#188

tbohn commented on drivers/classic/src/initialize_atmos.c in cea075a Jan 13, 2015

Again, why is sec declared as double here, when tmaxsec and tminsec are declared as ints?
UW-Hydro#188

else if (strcasecmp("TIME_STEP", optstr) == 0) {
sscanf(cmdstr, "%*s %u", &global_param.dt);
else if (strcasecmp("MODEL_STEPS_PER_DAY", optstr) == 0) {
sscanf(cmdstr, "%*s %zu", &global_param.model_steps_per_day);

This comment has been minimized.

@bartnijssen

bartnijssen Jan 13, 2015

Member

Should we make specifying MODEL_STEPS_PER_DAY the default option? It ensures that the number of time steps evenly fits into a day.

@bartnijssen

bartnijssen Jan 13, 2015

Member

Should we make specifying MODEL_STEPS_PER_DAY the default option? It ensures that the number of time steps evenly fits into a day.

This comment has been minimized.

@jhamman

jhamman Jan 13, 2015

Member

Yes, that is how I currently have it setup. I will raise an error if TIME_STEP or SNOW_STEP is used and direct the user to add the MODEL_STEPS_PER_DAY and SNOW_STEPS_PER_DAY.

@jhamman

jhamman Jan 13, 2015

Member

Yes, that is how I currently have it setup. I will raise an error if TIME_STEP or SNOW_STEP is used and direct the user to add the MODEL_STEPS_PER_DAY and SNOW_STEPS_PER_DAY.

This comment has been minimized.

@bartnijssen

bartnijssen Jan 14, 2015

Member

Excellent - that makes sense for this implementation.

@bartnijssen

bartnijssen Jan 14, 2015

Member

Excellent - that makes sense for this implementation.

This comment has been minimized.

@jhamman

jhamman Jan 14, 2015

Member

By the way, this is how it is done in CESM/RASM too. The timestep is derived from this integer parameter to insure dt fits evenly inside a day.

@jhamman

jhamman Jan 14, 2015

Member

By the way, this is how it is done in CESM/RASM too. The timestep is derived from this integer parameter to insure dt fits evenly inside a day.

}
if (global_param.endday == 0) {
log_err("Simulation end day has not been defined. Make sure "
"that the global file defines ENDDAY.");
}
else if (global_param.endday > lastday[global_param.endmonth]) {
else if (global_param.endday > lastday[global_param.endmonth - 1]) {
log_err("The specified simulation end day (%hu) > the number of "
"days in the ENDMONTH (%hu). Make sure that the global "
"file defines a positive integer for ENDDAY.",

This comment has been minimized.

@bartnijssen

bartnijssen Jan 13, 2015

Member

Why the -1? Has the range of global_param.endmonth changed from [0-11] to [1-12]?

@bartnijssen

bartnijssen Jan 13, 2015

Member

Why the -1? Has the range of global_param.endmonth changed from [0-11] to [1-12]?

This comment has been minimized.

@jhamman

jhamman Jan 13, 2015

Member

I think it has always been [1-12]. Looking at the current implementation, this may have been a existing bug.

@jhamman

jhamman Jan 13, 2015

Member

I think it has always been [1-12]. Looking at the current implementation, this may have been a existing bug.

@bartnijssen

This comment has been minimized.

Show comment
Hide comment
@bartnijssen

bartnijssen Jan 13, 2015

Member

There are a lot of whitespace-only changes in the commit that should not be included. Are you using the same uncrustify configuration file as we used before? You should rollback most of the whitespace only changes, cause they just clutter the commit and since uncrustify was already run on the original branch we should not include whitespace only changes.

Member

bartnijssen commented Jan 13, 2015

There are a lot of whitespace-only changes in the commit that should not be included. Are you using the same uncrustify configuration file as we used before? You should rollback most of the whitespace only changes, cause they just clutter the commit and since uncrustify was already run on the original branch we should not include whitespace only changes.

@jhamman jhamman self-assigned this Jan 15, 2015

@bartnijssen

This comment has been minimized.

Show comment
Hide comment
@bartnijssen

bartnijssen Jan 15, 2015

Member

The history file is opened (and time dimension defined) in:
initialize_history_file()
https://github.com/bartnijssen/VIC/blob/develop/drivers/image/src/vic_init_output.c

For the state file in (note this does not currently have a time dimension):
initialize_state_file()
https://github.com/bartnijssen/VIC/blob/develop/drivers/image/src/vic_store.c

For the reads, the time dimension is currently not treated explicitly (just read one slice at a time from the beginning of the file, which obviously needs work). I just assume that the time slice that is being read in vic_force() in https://github.com/bartnijssen/VIC/blob/develop/drivers/image/src/vic_force.c increments by 1 every time (dstart[0] increases, the rest is constant). Proper treatment of the calendar would mean that dstart[0] is calculated in a better way (I just start at 0 and increment).

On Jan 15, 2015, at 12:29 PM, Joe Hamman notifications@github.com wrote:

@bartnijssen , I added the initial mods for the image driver (5d2ad84). I think we will still want to update the netCDF time dimensions to use the new calendar functionality in vic_time(). I had trouble tracking down what you had put into the time variable in the history files so I left it as is for now. If you can point me to the right place, I'll add that as another commit.


Reply to this email directly or view it on GitHub.

Member

bartnijssen commented Jan 15, 2015

The history file is opened (and time dimension defined) in:
initialize_history_file()
https://github.com/bartnijssen/VIC/blob/develop/drivers/image/src/vic_init_output.c

For the state file in (note this does not currently have a time dimension):
initialize_state_file()
https://github.com/bartnijssen/VIC/blob/develop/drivers/image/src/vic_store.c

For the reads, the time dimension is currently not treated explicitly (just read one slice at a time from the beginning of the file, which obviously needs work). I just assume that the time slice that is being read in vic_force() in https://github.com/bartnijssen/VIC/blob/develop/drivers/image/src/vic_force.c increments by 1 every time (dstart[0] increases, the rest is constant). Proper treatment of the calendar would mean that dstart[0] is calculated in a better way (I just start at 0 and increment).

On Jan 15, 2015, at 12:29 PM, Joe Hamman notifications@github.com wrote:

@bartnijssen , I added the initial mods for the image driver (5d2ad84). I think we will still want to update the netCDF time dimensions to use the new calendar functionality in vic_time(). I had trouble tracking down what you had put into the time variable in the history files so I left it as is for now. If you can point me to the right place, I'll add that as another commit.


Reply to this email directly or view it on GitHub.

Joe Hamman
Merge branch 'develop' of github.com:UW-Hydro/VIC into feature/subhou…
…rly_timestep

Conflicts:
	drivers/image/src/get_global_param.c
	drivers/shared/include/vic_driver_shared.h
@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Jan 18, 2015

Member

Thanks @bartnijssen. In order to to keep this PR to a manageable size, I'm going to leave the updating of the image driver I/O time handling to another issue (#197) and PR .

Member

jhamman commented Jan 18, 2015

Thanks @bartnijssen. In order to to keep this PR to a manageable size, I'm going to leave the updating of the image driver I/O time handling to another issue (#197) and PR .

@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Jan 19, 2015

Member

I've completed my modifications for this branch. I've tested on a few grid cells with the classic driver only but it is running stably for all calendars. Remaining issues for the image driver have been opened so I think this can be merged then closed.

Member

jhamman commented Jan 19, 2015

I've completed my modifications for this branch. I've tested on a few grid cells with the classic driver only but it is running stably for all calendars. Remaining issues for the image driver have been opened so I think this can be merged then closed.

@bartnijssen

This comment has been minimized.

Show comment
Hide comment
@bartnijssen

bartnijssen Jan 20, 2015

Member

it is running stably for all calendars

Does that mean all the mtclim operations work correctly in all the different calendars (skipping leap days, etc) or simply that the model does not crash if we specify different calendars? The main concern with the different calendars is that this creates a lot of new options (and hence test pathways). If this has not been fully tested with mtclim (e.g. would that handle all the different calendars correctly - which is not the same as not crashing), then we need to disallow the calendars that have not been tested or only allow the calendars when all the forcings are fully specified.

Member

bartnijssen commented Jan 20, 2015

it is running stably for all calendars

Does that mean all the mtclim operations work correctly in all the different calendars (skipping leap days, etc) or simply that the model does not crash if we specify different calendars? The main concern with the different calendars is that this creates a lot of new options (and hence test pathways). If this has not been fully tested with mtclim (e.g. would that handle all the different calendars correctly - which is not the same as not crashing), then we need to disallow the calendars that have not been tested or only allow the calendars when all the forcings are fully specified.

@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Jan 20, 2015

Member

@bartnijssen:

That is a good point. The model does not crash with the different calendars.

Maybe @tbohn has some ideas on the applicability of other calendars to mtclim. I know there are a number of places where the code uses a hard coded year length but I seem to remember @tbohn thinking it would be ok for the standard, all_leap, and no_leap calendars. I think it would be an issue for the 360_day calendar.

I'm fine with restricting the use of other calendars to cases when mtclim is not used.

Member

jhamman commented Jan 20, 2015

@bartnijssen:

That is a good point. The model does not crash with the different calendars.

Maybe @tbohn has some ideas on the applicability of other calendars to mtclim. I know there are a number of places where the code uses a hard coded year length but I seem to remember @tbohn thinking it would be ok for the standard, all_leap, and no_leap calendars. I think it would be an issue for the 360_day calendar.

I'm fine with restricting the use of other calendars to cases when mtclim is not used.

@bartnijssen

This comment has been minimized.

Show comment
Hide comment
@bartnijssen

bartnijssen Jan 20, 2015

Member

Even a single leap day difference can add up to a month in a 100 year simulation, which is why this can come back to haunt us.

We'll want to include at the very least a warning with non-standard calendars that they have not been tested. I'd rather not rely on someone thinking it'd probably be OK, but prefer to do real testing. In the absence of that (I do not suggest making this a major push right now), keep the code, but warn the user and open a new issue for more thorough testing of this feature.

Bart

On Jan 20, 2015, at 10:46 AM, Joe Hamman notifications@github.com wrote:

@bartnijssen:

That is a good point. The model does not crash with the different calendars.

Maybe @tbohn has some ideas on the applicability of other calendars to mtclim. I know there are a number of places where the code uses a hard coded year length but I seem to remember @tbohn thinking it would be ok for the standard, all_leap, and no_leap calendars. I think it would be an issue for the 360_day calendar.

I'm fine with restricting the use of other calendars to cases when mtclim is not used.


Reply to this email directly or view it on GitHub.

Member

bartnijssen commented Jan 20, 2015

Even a single leap day difference can add up to a month in a 100 year simulation, which is why this can come back to haunt us.

We'll want to include at the very least a warning with non-standard calendars that they have not been tested. I'd rather not rely on someone thinking it'd probably be OK, but prefer to do real testing. In the absence of that (I do not suggest making this a major push right now), keep the code, but warn the user and open a new issue for more thorough testing of this feature.

Bart

On Jan 20, 2015, at 10:46 AM, Joe Hamman notifications@github.com wrote:

@bartnijssen:

That is a good point. The model does not crash with the different calendars.

Maybe @tbohn has some ideas on the applicability of other calendars to mtclim. I know there are a number of places where the code uses a hard coded year length but I seem to remember @tbohn thinking it would be ok for the standard, all_leap, and no_leap calendars. I think it would be an issue for the 360_day calendar.

I'm fine with restricting the use of other calendars to cases when mtclim is not used.


Reply to this email directly or view it on GitHub.

@tbohn

This comment has been minimized.

Show comment
Hide comment
@tbohn

tbohn Jan 20, 2015

Collaborator

Yes, I think the standard, no_leap and all_leap calendars would be fine
with MTCLIM - I believe MTCLIM uses a no_leap calendar and we just
duplicate a day for leap years.

I think we could even find a way to make the 360-day calendar work, if we
realy wanted to. But I am guessing that only the CCSM system uses it? And
if so, the forcings would already be supplied to VIC at sub-daily
timescales, so MTCLIM would be bypassed.

Ted

On Tue, Jan 20, 2015 at 10:46 AM, Joe Hamman notifications@github.com
wrote:

@bartnijssen https://github.com/bartnijssen:

That is a good point. The model does not crash with the different
calendars.

Maybe @tbohn https://github.com/tbohn has some ideas on the
applicability of other calendars to mtclim. I know there are a number of
places where the code uses a hard coded year length but I seem to remember
@tbohn https://github.com/tbohn thinking it would be ok for the standard,
all_leap, and no_leap calendars. I think it would be an issue for the
360_day calendar.

I'm fine with restricting the use of other calendars to cases when mtclim
is not used.


Reply to this email directly or view it on GitHub
#188 (comment).

Collaborator

tbohn commented Jan 20, 2015

Yes, I think the standard, no_leap and all_leap calendars would be fine
with MTCLIM - I believe MTCLIM uses a no_leap calendar and we just
duplicate a day for leap years.

I think we could even find a way to make the 360-day calendar work, if we
realy wanted to. But I am guessing that only the CCSM system uses it? And
if so, the forcings would already be supplied to VIC at sub-daily
timescales, so MTCLIM would be bypassed.

Ted

On Tue, Jan 20, 2015 at 10:46 AM, Joe Hamman notifications@github.com
wrote:

@bartnijssen https://github.com/bartnijssen:

That is a good point. The model does not crash with the different
calendars.

Maybe @tbohn https://github.com/tbohn has some ideas on the
applicability of other calendars to mtclim. I know there are a number of
places where the code uses a hard coded year length but I seem to remember
@tbohn https://github.com/tbohn thinking it would be ok for the standard,
all_leap, and no_leap calendars. I think it would be an issue for the
360_day calendar.

I'm fine with restricting the use of other calendars to cases when mtclim
is not used.


Reply to this email directly or view it on GitHub
#188 (comment).

@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Jan 20, 2015

Member

A cumulative leap day offset should not be a problem since mtclim only knows which day in the year [1, 366]. The only problem I see in mtclim is when there is a calculation that uses something like day_in_year / total_days_in_year. For example in calc_srad_humidity_iterative():

 /* begin loop through yeardays */
for (i = 0; i < DAYS_PER_YEAR; i++) {
    ...
     /* solar constant as a function of yearday (W/m^2) */
    sc = param.MTCLIM_SOLAR_CONSTANT + 45.5 *
    sin((2.0 * CONST_PI * (double)i / CONST_DDAYS_PER_YEAR) + 1.7);
    ....
}

In previous versions of mtclim, this was insensitive to whether or not this was a leap year.

Member

jhamman commented Jan 20, 2015

A cumulative leap day offset should not be a problem since mtclim only knows which day in the year [1, 366]. The only problem I see in mtclim is when there is a calculation that uses something like day_in_year / total_days_in_year. For example in calc_srad_humidity_iterative():

 /* begin loop through yeardays */
for (i = 0; i < DAYS_PER_YEAR; i++) {
    ...
     /* solar constant as a function of yearday (W/m^2) */
    sc = param.MTCLIM_SOLAR_CONSTANT + 45.5 *
    sin((2.0 * CONST_PI * (double)i / CONST_DDAYS_PER_YEAR) + 1.7);
    ....
}

In previous versions of mtclim, this was insensitive to whether or not this was a leap year.

@bartnijssen

This comment has been minimized.

Show comment
Hide comment
@bartnijssen

bartnijssen Jan 20, 2015

Member

@jhamman : If you add the small changes to the MTCLIM code that we discussed (pass both day of year and the number of days in the year), then I'll go ahead and merge

Member

bartnijssen commented Jan 20, 2015

@jhamman : If you add the small changes to the MTCLIM code that we discussed (pass both day of year and the number of days in the year), then I'll go ahead and merge

@bartnijssen

This comment has been minimized.

Show comment
Hide comment
@bartnijssen

bartnijssen Jan 20, 2015

Member

@jhamman : BTW - also note that the pull request cannot be merged automatically, because I merged another pull request first. If you can resolve the conflict on your end that would be great.

Member

bartnijssen commented Jan 20, 2015

@jhamman : BTW - also note that the pull request cannot be merged automatically, because I merged another pull request first. If you can resolve the conflict on your end that would be great.

@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Jan 20, 2015

Member

No problem, I'll bring this branch up to date before committing these last changes.

Member

jhamman commented Jan 20, 2015

No problem, I'll bring this branch up to date before committing these last changes.

Joe Hamman added some commits Jan 21, 2015

Joe Hamman
make mtclim somewhat calendar aware, mtclim control structure now use…
…s a parameter "days_per_year" to determine if leap days should be included.
Joe Hamman
Merge branch 'develop' of github.com:UW-Hydro/VIC into feature/subhou…
…rly_timestep

Conflicts:
	drivers/shared/include/vic_driver_shared.h
@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Jan 21, 2015

Member

Ok, with this last set of commits I

  • brought the branch up to date with the develop branch
  • applied the mtclim calendar fix we discussed today. This results in a no-change outcome for the standard, gregorian, and julian calendars where as the no_leap, all_leap and 360_day calendars are now handled in a better way.

The build passed and I verified in a few test cases that this is doing what we want.

Member

jhamman commented Jan 21, 2015

Ok, with this last set of commits I

  • brought the branch up to date with the develop branch
  • applied the mtclim calendar fix we discussed today. This results in a no-change outcome for the standard, gregorian, and julian calendars where as the no_leap, all_leap and 360_day calendars are now handled in a better way.

The build passed and I verified in a few test cases that this is doing what we want.

@bartnijssen

This comment has been minimized.

Show comment
Hide comment
@bartnijssen

bartnijssen Jan 21, 2015

Move hard-coded 360 to header (like DAYS_PER_LYEAR, etc).

This would be the last one - after that I will merge

Move hard-coded 360 to header (like DAYS_PER_LYEAR, etc).

This would be the last one - after that I will merge

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Jan 21, 2015

Owner

done in 87d1f04.

Owner

jhamman replied Jan 21, 2015

done in 87d1f04.

This comment has been minimized.

Show comment
Hide comment
@bartnijssen

bartnijssen Jan 21, 2015

bartnijssen replied Jan 21, 2015

bartnijssen added a commit that referenced this pull request Jan 21, 2015

@bartnijssen bartnijssen merged commit 6849a5b into UW-Hydro:develop Jan 21, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

This was referenced Jan 21, 2015

@jhamman jhamman deleted the jhamman:feature/subhourly_timestep branch Jan 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment