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

Fix indexing of layers in high freq. output #6497

Merged

Conversation

xylar
Copy link
Contributor

@xylar xylar commented Jul 3, 2024

Previously, the layer above the one containing the desired depth was being selected, rather than the layer containing the depth.

The default depth if no layer is found is now the deepest layer, rather than the first layer. This is because, if no layer is found, it means that even the deepest layer is shallower than the desired depth, and the only sensible default is the deepest layer.

non-BFB only for MPAS-Ocean high-frequency output.

Fixes #6496

Previously, the layer above the one containing the desired
depth was being selected, rather than the layer containing the
depth.

The default depth if no layer is found is now the deepest layer,
rather than the first layer.  This is because, if no layer is found,
it means that even the deepest layer is shallower than the desired
depth, and the only sensible default is the deepest layer.
@xylar xylar added mpas-ocean bug fix PR non-BFB PR makes roundoff changes to answers. labels Jul 3, 2024
@xylar
Copy link
Contributor Author

xylar commented Jul 3, 2024

I successfully ran ERS_Ld5.T62_oQU120.CMPASO-NYF.chrysalis_intel and added some temporary print statements, see:

/lcrc/group/e3sm/ac.xasay-davis/scratch/chrys/ERS_Ld5.T62_oQU120.CMPASO-NYF.chrysalis_intel.20240703_024722_lcosnw

With this fix, I'm seeing:

var                 i    refBottomDepth(i-1)     refBottomDepth(i)
-----------------------------------------------------------------
iLevel0100          26   97.8969986438751        106.103993177414
iLevel0250          38   237.887019395828        254.577030897141
iLevel0700          55   688.487065076828        729.107051134109
iLevel2000          75   1977.58716273308        2074.75707554817

So I do believe the fix is correct and the previous results were incorrect.

@xylar
Copy link
Contributor Author

xylar commented Jul 3, 2024

@jonbob, I've marked this as non-BFB because I assume high-frequency output is included in E3SM's checks. Could you verify this? And we presumably need to coordinate with @rljacob to find out if this should wait for the maint-3.0 branch or go in before that.

Copy link
Contributor

@vanroekel vanroekel left a comment

Choose a reason for hiding this comment

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

Approved by visual inspection and testing from @xylar. In addition to the bug fix I like the switch to the bottom if the layer is not found. Having a surface value in regions shallower than the target depth (as we do now) can lead to confusion in interpretation.

Thanks for the fixes @xylar!

@xylar
Copy link
Contributor Author

xylar commented Jul 3, 2024

Thanks @vanroekel!

@rljacob
Copy link
Member

rljacob commented Jul 3, 2024

If high-frequency output is in the 3.0 default output, then yes this should wait.

@xylar
Copy link
Contributor Author

xylar commented Jul 3, 2024

@rljacob, yes, it is.

@rljacob rljacob added this to the v3.1beta milestone Jul 3, 2024
@jonbob
Copy link
Contributor

jonbob commented Jul 3, 2024

@xylar -- high frequency output is not used in testing BFBness, but it is in the 3.0 default output. So it makes sense to wait

Copy link
Contributor

@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, I agree both based on the logic and your output.

jonbob added a commit that referenced this pull request Oct 21, 2024
#6497)

Fix indexing of layers in high freq. output

Previously, the layer above the one containing the desired depth was
being selected, rather than the layer containing the depth.

The default depth if no layer is found is now the deepest layer, rather
than the first layer. This is because, if no layer is found, it means
that even the deepest layer is shallower than the desired depth, and the
only sensible default is the deepest layer.

Fixes #6496

[non-BFB] only for MPAS-Ocean high-frequency output
@jonbob
Copy link
Contributor

jonbob commented Oct 21, 2024

Passes:

  • ERP_Ld3.ne30pg2_r05_IcoswISC30E3r5.WCYCL1850.chrysalis_intel.allactive-pioroot1

merged to next

@jonbob jonbob merged commit 1f51259 into E3SM-Project:master Oct 22, 2024
13 checks passed
@jonbob
Copy link
Contributor

jonbob commented Oct 22, 2024

merged to master

@rljacob
Copy link
Member

rljacob commented Oct 22, 2024

I'm not sure why this was labeled v3.1beta. There weren't any diffs to bless.

@rljacob
Copy link
Member

rljacob commented Oct 22, 2024

Is the high-frequency output not in our production tests? It should be.

@jonbob
Copy link
Contributor

jonbob commented Oct 22, 2024

@rljacob -- those files are produced by default in our production tests, but no current testing does cprnc comparisons of mpaso output files

@jonbob
Copy link
Contributor

jonbob commented Oct 22, 2024

@rljacob -- I think is was labeled v3.1beta because it would change output for any of the ongoing v3 campaigns, even if it didn't change the state as seen by the coupler

@rljacob
Copy link
Member

rljacob commented Oct 22, 2024

Its changing a diagnostic quantity and not the prognostic state (or it would cause a diff in the coupler output). Thats a gray area for versioning.

@jonbob
Copy link
Contributor

jonbob commented Oct 22, 2024

@rljacob -- agreed. We were worried about adding anything that could make differences in the DECK run, even just diagnostic files

@rljacob
Copy link
Member

rljacob commented Oct 22, 2024

I'm looking at the v3 output and see a file with string "mpaso.hist.am.highFrequencyOutput". Is that the only one affected by this change?

@jonbob
Copy link
Contributor

jonbob commented Oct 22, 2024

@rljacob -- yes, that should be it

xylar added a commit to xylar/compass that referenced this pull request Oct 26, 2024
This merge updates the E3SM-Project submodule from [727ad81](https://github.com/E3SM-Project/E3SM/tree/727ad81) to [1442143](https://github.com/E3SM-Project/E3SM/tree/1442143).

This update includes the following MPAS-Ocean and MPAS-Frameworks PRs (check mark indicates bit-for-bit with previous PR in the list):
- [ ]  (ocn) E3SM-Project/E3SM#6509
- [ ]  (ocn) E3SM-Project/E3SM#6508
- [ ]  (fwk) E3SM-Project/E3SM#6575
- [ ]  (ocn) E3SM-Project/E3SM#6590
- [ ]  (fwk) E3SM-Project/E3SM#6643
- [ ]  (ocn) E3SM-Project/E3SM#6656
- [ ]  (ocn) E3SM-Project/E3SM#6672
- [ ]  (ocn) E3SM-Project/E3SM#6659
- [ ]  (ocn) E3SM-Project/E3SM#6497
- [ ]  (ocn) E3SM-Project/E3SM#6485
- [ ]  (ocn) E3SM-Project/E3SM#6566
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix PR mpas-ocean non-BFB PR makes roundoff changes to answers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in finding layers with given depths in MPAS-Ocean high-frequency output
5 participants