Skip to content

Conversation

@xylar
Copy link
Collaborator

@xylar xylar commented Jan 11, 2018

To save the bother of writing out the MHT stream just for the latitude bins, these bins can now (optionally) be read from the timeSeriesStatsMonthlyOutut stream instead.

The refZMid, refBottomDepth and refThickness are read or computed from a restart file instead of the MHT stream.

@xylar xylar self-assigned this Jan 11, 2018
@xylar xylar requested a review from mark-petersen January 11, 2018 03:09
@xylar xylar added the clean up label Jan 11, 2018
@xylar
Copy link
Collaborator Author

xylar commented Jan 11, 2018

@mark-petersen, I know we don't have any output with the latitude bins in timeSeriesStatsMonthlyOutput to test this on yet. Please point me to it once we do.

In the meantime, you can make sure that an existing run of your choice still produces the MHT plot as before.

@xylar
Copy link
Collaborator Author

xylar commented Jan 11, 2018

Testing

I tested on the QU240 run on my laptop. Here is the standard MHT plot from this branch:
mht_gmpas-qu240_years0002-0005
and from develop:
mht_gmpas-qu240_years0002-0005
There is no appreciable difference.

Here is the plot including depth from this branch (turned off in my run with develop):
mhtz_gmpas-qu240_years0002-0005

Copy link
Collaborator

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

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

Yes, this is perfect. Since it is optional it is best to put it in now. Thanks for your quick action and testing here. Please merge.

@milenaveneziani
Copy link
Collaborator

We should have the test run with the new output sometime today, so we can test this then.

@xylar
Copy link
Collaborator Author

xylar commented Jan 12, 2018

Yes, we need to test this on the new output once it's available. Merging now an fixing later doesn't really save us any time.

@xylar xylar mentioned this pull request Jan 14, 2018
15 tasks
To save the bother of writing out the MHT stream just for the
latitude bins, these bins can now (optionally) be read from the
timeSeriesStatsMonthlyOutut stream instead.

The refZMid, refBottomDepth and refThickness are read or computed
from a restart file instead of the MHT stream.
@xylar xylar force-pushed the update_mht_task_data_sources branch from b7fc73d to 0d3b64c Compare January 23, 2018 02:39
@xylar
Copy link
Collaborator Author

xylar commented Jan 23, 2018

@milenaveneziani, do you remember what the status of testing this PR is? I lost track of it.

@milenaveneziani
Copy link
Collaborator

milenaveneziani commented Jan 23, 2018

We do have the new data to test it with.
This can go in independently from #294, right?

@xylar
Copy link
Collaborator Author

xylar commented Jan 23, 2018

Testing Continued

For the first 2 years of a QU240 run, everything seems to work both for MPAS-Analysis/develop and for this branch.

develop: http://portal.nersc.gov/project/acme/xylar/a24w_develop/

new branch: http://portal.nersc.gov/project/acme/xylar/a24w_update_source/

I also ran a test where I manually made sure the code doesn't use the MHT stream and that also worked fine. So I think MPAS-Analysis should work fine with these changes.

@xylar
Copy link
Collaborator Author

xylar commented Jan 23, 2018

This can go in independently from #294, right?

Yes, the 2 PRs are independent so we can merge as soon as we're confident in the results. Which I think we are.

@xylar xylar merged commit 64ff5c0 into MPAS-Dev:develop Jan 23, 2018
@xylar xylar deleted the update_mht_task_data_sources branch January 23, 2018 19:39
@xylar xylar mentioned this pull request Jan 26, 2018
xylar added a commit that referenced this pull request Jan 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants