-
Notifications
You must be signed in to change notification settings - Fork 395
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
base: develop
Are you sure you want to change the base?
fixes for exact restarts and a few lines of cleanup #873
Conversation
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.
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]); |
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.
Why did you change the +=
to =
here?
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 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] / |
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.
Why did you change the +=
to =
here?
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.
Same as above - this is leftover from when we were aggregating over bands.
@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 I also agree that specifying the literals is unlikely to solve the problem. |
Agreed on the |
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 invic_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 withincesm_put_data
for these fields, this isn't needed anymore since we reconfigured this to use fields directly fromout_data
.