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

fixes for exact restarts and a few lines of cleanup #873

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

dgergel
Copy link
Contributor

@dgergel dgergel commented May 14, 2019

This PR is intended to fix our current problem with exact restarts in RASM with VIC5. For additional reference for details on which global sums are different, see #872.

The solution proposed here is explicit typecasting to double as we currently do in vic_store for writing the model state. Here I have only explicitly typecasted the fields that are failing exact restarts, we may want to do this for all fields (??).

This PR also includes two minor changes that I noticed while doing the typecasting - for latent and sensible heat we had kept the += which was a relic from when we added over bands within cesm_put_data for these fields, this isn't needed anymore since we reconfigured this to use fields directly from out_data.

@dgergel dgergel requested a review from bartnijssen May 14, 2019 21:23
@dgergel dgergel self-assigned this May 14, 2019
Copy link
Member

@bartnijssen bartnijssen left a comment

Choose a reason for hiding this comment

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

I am not convinced that the type-casting of the entire quantity on the RHS makes a difference, since almost all the elements on the RHS are already doubles, but it may be worth a try. However, I would probably not do the typecast since I think that it may be hiding something else. The more likely reason would be a typecast when we write to or read from the state file (for example, a value that is internally represented as a double it written or read as a float (loss-of-precision). Another thing would be just to specify that the literals (-1, 2) should be doubles. So just replace -1 by -1. and 2 by 2., but I still doubt that will make a difference.

My suggestion: try it out and in the meantime make sure that when the actual state is written and read that we use all doubles. If that is still not it, then the question is whether some of the values here are not properly initialized when we read the statefile. For some of these it would not matter in the offline simulation. My guess is actually that that is the most likely cause.


// sensible heat, VIC: W/m2, CESM: W/m2
l2x_vic[i].l2x_Fall_sen += -1 * out_data[i][OUT_SENSIBLE][0];
l2x_vic[i].l2x_Fall_sen = (double) (-1 * out_data[i][OUT_SENSIBLE][0]);
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change the += to = here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was leftover from when we were aggregating over bands - since we don't do that anymore, there's no reason to have the +=.


// evaporation, VIC: mm, CESM: kg m-2 s-1
l2x_vic[i].l2x_Fall_evap += -1 * out_data[i][OUT_EVAP][0] /
global_param.dt;
l2x_vic[i].l2x_Fall_evap = (double) (-1 * out_data[i][OUT_EVAP][0] /
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change the += to = here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above - this is leftover from when we were aggregating over bands.

@dgergel
Copy link
Contributor Author

dgergel commented Jun 18, 2019

@bartnijssen I don't think that the culprit is reading from/to a statefile, since we fixed exact restarts a while back in the image driver. I think what we're dealing with is purely within the CESM driver.

I think properly initializing the l2x fields could be an issue. See ~/vic/drivers/cesm/src/vic_cesm_init_library.c for how we are initializing the l2x fields with SHR_CONST_SPVAL.

I also agree that specifying the literals is unlikely to solve the problem.

@bartnijssen
Copy link
Member

Agreed on the l2x fields I will be looking at those. I am going to leave the PR open for the time being till I have had a chance to look in more detail at the initialization of the l2x fields. For the cesm driver we may need to add some additional fields to the state files for exact restarts.

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

Successfully merging this pull request may close these issues.

None yet

2 participants