-
Notifications
You must be signed in to change notification settings - Fork 385
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
Adds submesoscale parameterization for mpas-ocean #5099
Conversation
This PR has been tested in a fully coupled configuration for 200+ years on chrysalis. The simulation page describing the run is here Direct link to the diagnostics are here |
mocStreamValLatAndDepthRegionLocal(1, k - 1, iTransect) & | ||
+ sumTransport(k - 1, iTransect) | ||
mocStreamValLatAndDepthRegionLocal(1, k + 1, iTransect) & | ||
- sumTransport(k + 1, iTransect) |
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.
Note that this change came from a discussion with Gokhan about how to calculate AMOC. If preferred I can make this one change its own PR.
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.
Sorry for being nosy, but how do the old and new way of computing AMOC affect the AMOC curves you show above?
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.
No problem at all. Thanks for looking. The change ends up being about 0.5-1 Sv on average increase, but not every month, some months go up some down.
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.
So summing from the bottom up? Could you explain a bit more about the k index change? I don't fully follow how the sign change and the index change relate to each other. Does the sign of the resulting MOC somehow remain the same?
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 think it would be better to make this its own PR and explain the change further. We would want to make the corresponding change in MPAS-Analysis and do some tests to make sure the two are consistent.
do iEdge=1,nEdgesAll | ||
cell1 = cellsOnEdge(1,iEdge) | ||
cell2 = cellsOnEdge(2,iEdge) | ||
indMLDedge(iEdge) = min(indMLD(cell1),indMLD(cell2)) |
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.
@philipwjones I think I could move this to a calculation in the submesocale eddy calculation to remove this from ocn_diagnostics_variables if you prefer that approach.
Here is relevant PACE data for the runs -- https://pace.ornl.gov/search/20220715.submeso.piControl.ne30pg2_EC30to60E2r2.chrysalis |
@vanroekel the CI build is failing because you've accidentally included submodule updates to the land component in this PR. |
Thanks @ambrad I think I got all the unintended commits removed from the PR. The CI passes now. |
@vanroekel Thank you Luke for sharing the feature looks good. From a user side minor comment (or more question), apologies if I missed the information. I see the submesoscale parameterization settings are in default Registry, does this mean this feature will be turned on as a default when setting up a case (for me g-case)? |
@ytakano3 yes for now we will leave this default off. But all you have to do is add
in your |
@vanroekel Thank you Luke, that is straightforward. |
@vanroekel can you change these lines so that this compiles with OpenMP? I do not have permissions to push:
|
A 5-day master pre- vs. post-merge comparison on Summit+pgigpu with the streams.ocean mod shows 1 diff in mpaso.hist.am.highFrequencyOutput.0001-01-01_00.00.00.nc:
Run-dir on Summit:
|
Thanks. Do you know how to interpret these lines? Are there only differences in six locations, and lines 2 and 3 here are the diffs at those index locations? Why does the second line have six values and the third have four?
|
@mark-petersen - the cprnc output is difficult to read, but it means there are differences in 11845131 of 14344760 values. Lines 2 and 3 are for the extrema in values: line 2 has max and min values from file1 (I think) as well as max diff (3.3e-2), the file1 value at the max diff location, the max relative diff, and the file1 value at that location. Line 3 does not repeat the max diff values but otherwise outputs the same information from file2 |
That's really strange. It seems like |
As of E3SM-Project/E3SM#5099, this variable is no longer part of the AM and is instead part of the main MPAS-Ocean code.
I agree. 11.3M of 14.3M values of the array mismatch, so this is not on inconsequential cells like land cells as we thought. But |
Just a note that I don't think What is especially odd to me is that this PR did not change the calculation of |
Thanks @vanroekel, I think that at least helps us understand why I think we're in a pinch in therms of investigating further because those of us involved in this PR don't seem to have RSA tokens for Summit or Ascent. Am I incorrect about that? |
I tried to narrow down: CPU runs with |
That's a good hint, @amametjanov. I haven't done much with !$acc pragmas, but this loop in mpas_ocn_diagnostics.F (lines 1873-1919) looks suspicious to me, like div_hu and div_huTransport are missing from the private section?
|
!$acc vertTransportVelocityTop, vertGMBolusVelocityTop, invAreaCell, & | ||
!$acc minLevelCell) & | ||
!$acc normalTransportVelocity, vertVelocityTop, vertTransportVelocityTop, & | ||
!$acc invAreaCell, minLevelCell) & |
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.
Yes, these lines were changed in this PR.
!$acc vertTransportVelocityTop, vertGMBolusVelocityTop, invAreaCell, & | ||
!$acc minLevelCell) & | ||
!$acc normalTransportVelocity, vertVelocityTop, vertTransportVelocityTop, & | ||
!$acc invAreaCell, minLevelCell) & | ||
!$acc private(invAreaCell1, i, iEdge, edgeSignOnCell_temp, dcEdge_temp, & | ||
!$acc dvEdge_temp, k, r_tmp) |
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.
But the private lines were not changed - div_hu
and div_huTransport
should be private and listed here, but they were not listed before this PR either. Regardless, they should be added.
Since they are not listed as private, this calculation could simply be wrong for OpenACC, but wrong in different ways before and after this PR.
Yes, the present() clause is missing both a maxLevelCell and layerThickEdgeFlux and the div_huGMBolus needs to be added to the private clause. The latter is likely causing some race conditions. The former are just potentially leading to unnecessary data motion. Similar changes are needed in the sister routine diagnostic_solve_MLEvel and the original diagnostic_solve_vortVel. Not sure how the original evaded detection... |
…5167) Add normalGMBolusVelocity to the gm package This PR is needed to put the normalGMBolusVelocity in the gm package, so it is not allocated when GM is off. Otherwise, extra memory is allocated unnecessarily for high-resolution simulations. This reverts commit 2c6f496, which was a small change in #5099 to make that PR BFB on intel-19 optimized in MPAS-Ocean standalone. This PR is BFB with gcc-9 debug, gcc-9 optimized, and intel-19 debug. For intel-19 optimized there is a mismatch with master of 1e-12 on the first timestep for the MPAS-Ocean standalone compass test ocean_global_ocean_QU240_PHC_performance_test on chrysalis. It appears that intel-19 conducts some optimization that changes the order of operations for the lines involved. We expect this PR to be bfb for all E3SM tests. [BFB]
@philipwjones - can I make an issue from this and assign you or @amametjanov? |
@jonbob - yes, and go ahead and assign me since I need to fix some device_resident code in the same routines |
moving conversation to E3SM Issue #5176, since this PR has already been merged |
Add normalGMBolusVelocity to the gm package This PR is needed to put the normalGMBolusVelocity in the gm package, so it is not allocated when GM is off. Otherwise, extra memory is allocated unnecessarily for high-resolution simulations. This reverts commit 2c6f496, which was a small change in #5099 to make that PR BFB on intel-19 optimized in MPAS-Ocean standalone. This PR is BFB with gcc-9 debug, gcc-9 optimized, and intel-19 debug. For intel-19 optimized there is a mismatch with master of 1e-12 on the first timestep for the MPAS-Ocean standalone compass test ocean_global_ocean_QU240_PHC_performance_test on chrysalis. It appears that intel-19 conducts some optimization that changes the order of operations for the lines involved. We expect this PR to be bfb for all E3SM tests. [BFB]
…on' into next (PR #5172) Enables and modifies the submesoscale eddy parameterization This enables the Fox-Kemper et al 2011 submesoscale eddy parameterization as the default for all configurations. It also fixes a unit error in the submesoscale eddy code that did not influence the long control run, but was introduced in the first PR (#5099). The variable gradBuoyEddy was not the buoyancy but the density gradient. Here the name is changed to gradDensityEddy for clarity. [NML] [CC]
…on' (PR #5172) Enables and modifies the submesoscale eddy parameterization This enables the Fox-Kemper et al 2011 submesoscale eddy parameterization as the default for all configurations. It also fixes a unit error in the submesoscale eddy code that did not influence the long control run, but was introduced in the first PR (#5099). The variable gradBuoyEddy was not the buoyancy but the density gradient. Here the name is changed to gradDensityEddy for clarity. [NML] [CC]
Adds the Fox-Kemper et al 2011 parameterization for submesocale eddies.
There is also a substantial eddy parameterization reorganization as the
buoyancy gradient that was calculated within GM is also needed for this
parameterization. There are also cases when we will run with the
submeso parameterization and not GM so that calculation is separate.
The density threshold MLD is also removed from the analysis member as it
is required by numerous calculations in the forward model now.
The contributions of submesoscale eddy velocity to AMOC is also added to
the MOC streamfunction computation
Description of new parameterization is as follows. This appears in the file
components/mpas-ocean/src/shared/mpas_ocn_submesoscale_eddies.F
.[NML]
[BFB] - stealth feature not turned on by default