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

Feature/debug cesm driver #642

Merged
merged 36 commits into from
Jan 6, 2017

Conversation

dgergel
Copy link
Contributor

@dgergel dgergel commented Oct 8, 2016

No description provided.

Joe Hamman and others added 16 commits September 5, 2016 21:49
* 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
* 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
(Fixing a problem with image restarts when using multiple processors).

/******************************************************************************
* @brief Save model state.
*****************************************************************************/
void
vic_store(dmy_struct *dmy_current,
case_metadata *cmeta,
Copy link
Member

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>
Copy link
Member

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.

Copy link
Contributor Author

@dgergel dgergel Oct 9, 2016

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
Copy link
Member

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
Copy link
Member

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)
Copy link
Member

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.
Copy link
Member

@jhamman jhamman left a 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);


Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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));

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

dgergel and others added 6 commits October 11, 2016 16:34
…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
@bartnijssen
Copy link
Member

@jhamman : Can you review and merge when ready?

Copy link
Member

@jhamman jhamman left a 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));

Copy link
Member

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);


Copy link
Member

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.

@bartnijssen
Copy link
Member

@dgergel : Please complete so @jhamman can merge

@dgergel
Copy link
Contributor Author

dgergel commented Jan 4, 2017

@jhamman I'll tie this up today.

Copy link
Contributor Author

@dgergel dgergel left a 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));

Copy link
Contributor Author

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);


Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@jhamman jhamman merged commit d1872e7 into UW-Hydro:develop Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants