-
Notifications
You must be signed in to change notification settings - Fork 397
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
Feature/debug cesm driver #642
Conversation
update command line usage docs
* Fixed image driver history file name timetamp - now is timstep-beginning * Added release notes about PR UW-Hydro#635 (fixing image driver history filename timestamp)
Added an if statement so that when VIC is run on multiple nodes, only the master node will validate state file dimensions and coordinate variables.
Use multiple processors for image restart tests
Fixing a problem with image restarts when using multiple processors
…communicating with the coupler
* Fixed a bug when calculating steps of forcing to skip - should round to integer first before converting to int type to prevent possible 1-timestep wrong offset * Added bug fix release notes to PR#639
…an int in gather_put_nc_field_int fctn
(Fixing a problem with image restarts when using multiple processors).
|
||
/****************************************************************************** | ||
* @brief Save model state. | ||
*****************************************************************************/ | ||
void | ||
vic_store(dmy_struct *dmy_current, | ||
case_metadata *cmeta, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't want to pass case_metadata
in to vic_store
. Instead, you should set filenames.statefile = cmeta->caseid
.
@@ -25,12 +25,14 @@ | |||
*****************************************************************************/ | |||
|
|||
#include <vic_driver_shared_image.h> | |||
#include <vic_driver_cesm.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file is also compiled with the image driver so you need to find a solution that does not require the cesm driver. I think that all you need is the caseid which can be stored as filenames.statefile
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - I'm doing this now in the vic_cesm_run
function.
// assign case name to state file name | ||
strncpy(filenames.statefile, trim(cmeta->caseid), | ||
sizeof(filenames.statefile)); | ||
// write state file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have the right idea here, but we should just put this logic in vic_cesm_start
since we have all this info at initialization.
USE, INTRINSIC :: ISO_C_BINDING | ||
USE vic_cesm_def_mod | ||
IMPLICIT NONE | ||
TYPE(vic_clock), INTENT(in) :: vclock | ||
TYPE(case_metadata), INTENT(in) :: cmeta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can roll back these changes as well.
@@ -381,7 +381,7 @@ SUBROUTINE lnd_run_mct(EClock, cdata, x2l, l2x, cdata_s, x2s, s2x) | |||
CALL lnd_import_mct(x2l) | |||
|
|||
!--- run vic | |||
errno = vic_cesm_run(vclock) | |||
errno = vic_cesm_run(vclock, cmeta) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
roll back these changes.
…ic_cesm_run function because adding this logic to vic_cesm_init instead" This reverts commit 7e28f13.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting close. Can you also add an entry to the change log?
global_param.calendar, global_param.time_units, | ||
&end_time_date); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block looks good but can you move it into the if (mpi_rank == VIC_MPI_ROOT)
block below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, should I also put this new block into vic_cesm_start
? Since this function is in /shared_image
, we actually want the state file info coming from the global parameter file unless we're running the CESM driver...It'd require passing dmy_current
into vic_cesm_start
but seems more internally consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, on second thought, the timestamp will be incorrect if it's set at initialization and not at the time at which we write the state file. But I still think it seems like this shouldn't be in vic_store
, since that's a shared_image function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be addressed still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
// assign case name to state file name | ||
strncpy(filenames.statefile, trim(cmeta->caseid), | ||
sizeof(filenames.statefile)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please put this in vic_cesm_start()
. As you can see, we don't really do anything in this top level function except call other functions and I'd like to keep it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still needs to be addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
…er type (UW-Hydro#645) * Added check to ensure mask varible in domain file is integer type * Cleaned up some printing lines * Added get_nc_var_type.c * Minor update of comments and ReleaseNotes * Added domain file description in docs * Small fix of table in docs * Added ncdump -h results for domain file in docs * Minor docs update
* Added check to ensure mask varible in domain file is integer type * Cleaned up some printing lines * Added get_nc_var_type.c * Minor update of comments and ReleaseNotes * Added domain file description in docs * Small fix of table in docs * Added ncdump -h results for domain file in docs * Minor docs update * The input arguments to function `make_lastday` are sometimes in a wrong order - fixed the bug * Added ReleaseNotes regarding PR#647
@jhamman : Can you review and merge when ready? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still a few changes hanging out there. We also need a changelog entry.
// assign case name to state file name | ||
strncpy(filenames.statefile, trim(cmeta->caseid), | ||
sizeof(filenames.statefile)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still needs to be addressed.
global_param.calendar, global_param.time_units, | ||
&end_time_date); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be addressed still.
@jhamman I'll tie this up today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhamman, I've made the requested changes.
// assign case name to state file name | ||
strncpy(filenames.statefile, trim(cmeta->caseid), | ||
sizeof(filenames.statefile)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
global_param.calendar, global_param.time_units, | ||
&end_time_date); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
No description provided.