-
Notifications
You must be signed in to change notification settings - Fork 371
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
Fix two bugs in timeSeriesStats output in MPAS-Ocean #5218
Fix two bugs in timeSeriesStats output in MPAS-Ocean #5218
Conversation
This PR is based off of #5202 and will need to be rebased after that goes in. |
TestingI have used the 4 new tests introduced in MPAS-Dev/compass#435 to show that these fixes work as expected in standalone MPAS-Ocean and to demonstrate the bugs using the branch in #5202. |
@jonbob, would you be willing to run a 1-year E3SM simulation with this branch that you can compare with something you already have handy? That should be plenty to verify that |
@mark-petersen, I'd like this fix to go in before the code freeze. Any chance you could review it soon? |
@matthewhoffman, your suggested fix of using |
Thanks, I'm reviewing now. MPAS-Seaice has the identical timeSeriesStats. Does it need the identical fix? |
Yes, eventually, MPAS-Seaice will need this and also the equivalent of #5202. It seems hard to get both of those in and tested before the code freeze, though. |
I tried an E3SM test ( |
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.
Tested with gnu debug, gnu optimized, and intel optimized using all daily and monthly timeSeriesStats tests and MPAS-Dev/compass#435. This PR passes all. Based on @xylar's comments, it looks like this fixes the problem described.
b86244a
to
d063071
Compare
Update: |
d063071
to
774b398
Compare
note: running this PR in E3SM did not initially achieve the expected results |
Yes, I'm working on debugging this. It is proving quite challenging. |
1857ba2
to
83c98e3
Compare
@mark-petersen and @jonbob, I have updated the PR description and requested a re-review. |
if ($CONTINUE_RUN eq 'TRUE') { | ||
add_default($nl, 'config_AM_timeSeriesStatsMonthly_compute_on_startup', 'val'=>".false."); | ||
} else { | ||
add_default($nl, 'config_AM_timeSeriesStatsMonthly_compute_on_startup', 'val'=>".true."); | ||
} |
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 and similar changes above and below for other time-series stats seems to be necessary so that the first day or month in an E3SM simulation is complete. Let me know if you have concerns.
description="Logical flag determining if an analysis member computation occurs on start-up. You likely want this off for this (time series) analysis member because it will accumulate any state prior to time stepping (double counting the last time step)." | ||
description="Logical flag determining if an analysis member computation occurs on start-up. This should be set to .false. for better efficiency in most cases because the results of this first call will simply be thrown away. The exception is at the beginning of new E3SM simulations (CONTINUE_RUN = FALSE), where the first time step will be missing if this is not set to .true." |
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 and related descriptions for other time-series stats have been updated for better accuracy and clarity. @jonbob, I didn't try to update the associate files in bld
because I don't remember the exact process. I can look that up, though, if you would like me to take care of it.
Re-testingI again ran the 4 new tests introduced in MPAS-Dev/compass#435 to show that these fixes work as expected in standalone MPAS-Ocean. I also ran 3 months of the E3SM test case
See results in:
|
Getting there but testing in MPAS-Seaice is still showing problems that I want to understand before merging this. |
telecon notes: waiting for @xylar to fix a few things. |
Still debugging... |
I looked into this further today. Similar fixes in MPAS-Seaice revealed a problem with this approach. I have an alternative in mind that I will try to implement next week. If this PR being open is a problem, we can close it for now and re-open later. |
Before this merge, it was incorrectly assumed to be one time step before the current time. The correct interval depends on the compute interval, which has now been made available in the form of the `computeInterval`.
These variables will get read back on restart into the main system versions of these variables, which is not what we would want. Instead, on restart, Time_bnds(1) needs to be recomputed from xtime_start
Previously, timeSeriesStats was using the start time of the current run as the beginning of stats. Since this time starts over at restarts, it was not triggering a reset of the stats as it should have at the beginning of a restart interval that is commensurate with the time-averaging interval. With this merge, we now use the `simulationStartTime` to determine when time-averaging began, so that alarms are correctly triggered on restart.
With this merge, we copute timeSeriesStats on startup if we are doing a fresh E3SM run but not if we are restarting. This ensures that we have a complete first day, month, etc.
83c98e3
to
7e1a598
Compare
notes: had to be redone. Still in progress. |
I can close this for now as well. |
This merge fixes two issues in
timeSeriesStatsDaily
andtimeSeriesStatsMonthly
in MPAS-Ocean.The first is that the value
xtimeMonthly_start
was inaccurate at the start of a simulation and after model restart. This occurred because of a combination of problems.xtimeMonthly_start
was being computed by subtracting a time step from the current time, rather than a compute interval oftimeSeriesStats
after a restart.This issue was present in E3SM output.
closes #5217
The second issue does not affect E3SM output because E3SM doesn't use restart files for
timeSeriesStats
. However, standalone runs that do use restart files for this analysis were experiencing a problem in which time averaging was not getting reset after a restart, leading to time averages over the whole simulation rather than the expected (daily or monthly) interval. This issue has been fixed by referencing the reset alarm to thesimulationStartTime
rather than the beginning of the current (possibly restart) run.closes MPAS-Dev/MPAS-Model#121