Fixes indexing issue in SurfaceAreaWeighted AM #173

vanroekel · 2019-03-05T05:24:23Z

Nothing is presently being stored in workArray(1,:) leading to all
values in the var_array being shifted by one index. This is causing
MPAS-Analysis failures. This results in avgSurfaceTemperature values
being stored in avgSurfaceSalinity in output files.

This commit shifts indices back one value so the ordering is as
expected.

vanroekel · 2019-03-05T05:26:16Z

There is an issue in MPAS-analysis with time series analysis in the ocean. The surface area averaged SST does not look correct, e.g. see here

https://portal.nersc.gov/project/acme/lvroekel/20181129.Langmuir-LF17.A_WCYCL1850S.ne30_oECv3_ICG.anvil_years1-20/ocean/index.html#timeseries&gid=16&pid=2

in newer versions of the code, the workArray variable is started at 2 instead of 1 as in earlier versions of the code. This PR fixes this discrepancy.

milenaveneziani · 2019-03-05T05:29:00Z

src/core_ocean/analysis_members/mpas_ocn_surface_area_weighted_averages.F

good catch @vanroekel. But didn't the n=1 on the previous line help initialize workArray(n,:) correctly?

it only initialized the 1st index, previously all indices were initialized to zero. I've reverted now to using

workArray(:,:) = 0.0_RKIND

which should initialize the whole array to zero and not just workArray(1,:)

I see. I don't even remember when this analysis member was changed. Anyways, thanks for finding this.

xylar

This fix looks right to me. I'm surprised that we just noticed it now, given that the code changes are from 6 months ago.

xylar · 2019-03-05T06:09:17Z

So if I understand the error correctly, the plots are actually showing a time series of global mean surfaceThicknessFlux? https://github.com/MPAS-Dev/MPAS-Model/pull/173/files#diff-86aff6b62090a3bde2d3bdfcbe934259L390

milenaveneziani · 2019-03-05T06:13:15Z

I'm surprised that we just noticed it now, given that the code changes are from 6 months ago.

because probably most simulations we have looked at recently were run with an older version of the code.. We finally noticed when @darincomeau and Gennaro used the most recent code for the newer simulations. Glad it didn't affect the larger simulation campaigns.

mark-petersen · 2019-03-05T13:58:40Z

src/core_ocean/analysis_members/mpas_ocn_surface_area_weighted_averages.F

You are removing an index advancement here. Were all the variables off by an index of one before?
For example, latentHeatFlux was put in workArray(4,:) and will now be put in workArray(3,:). Do we need to make a corresponding index adjustment somewhere else?

Yes, everything is off by one index. Currently, we only use SST in MPAS analysis so that's the only one we noticed.

I see. I've answered my own question. This is the original in 0b984bd

src/core_ocean/analysis_members/mpas_ocn_surface_area_weighted_averages.F 345 ! copy data into work array 346 workArray( :,:) = 0.0_RKIND 347 workArray( 1,:) = workMask(:) 348 workArray( 2,:) = areaCell(:) 349 if (activeTracersBulkRestoringPKG) workArray( 3,:) = latentHeatFlux(:)

and the bug was introduced in 4b64b2d. So this PR just restores the indexing back to the original.

mark-petersen

Thanks for fixing this @vanroekel

mark-petersen · 2019-03-05T14:29:42Z

This will need to be put on maint-1.1 and maint-1.2.

xylar · 2019-03-05T14:34:13Z

We will also need to decide how to proceed with the Cryosphere simulations. We could put a hack in MPAS-Analysis if we don't want to start anything (particularly the mid-res) over.

xylar · 2019-03-05T14:35:58Z

@matthewhoffman, you are marked as a reviewer here because the code being modified was yours. I think this is a pretty high priority to fix so please take a look when you can.

xylar · 2019-03-05T14:42:18Z

This will need to be put on maint-1.1 and maint-1.2.

So I think this needs some discussion. The trick that @mark-petersen used to merge to maint-1.2 the last time is kind of a one-off. There are now 2 copies of the same commit in the history of e3sm/develop, which seems non-ideal to me. Maybe there's no good way around this, though.

milenaveneziani · 2019-03-05T14:43:11Z

We could put a hack in MPAS-Analysis if we don't want to start anything (particularly the mid-res) over

Yes, I'd say let's do that for the simulation that has already started (@darincomeau can do that on his local branch of mpas-a), and then update to this fix for the newer simulations.

mark-petersen · 2019-03-05T14:54:44Z

Yes, since we are just off by an index, it is easier to adjust MPAS-Analysis, rather than have an index change part-way through by changing MPAS-Ocean code for a particular simulation already underway.

vanroekel · 2019-03-05T14:58:53Z

To get this to work in mpas-analysis, I think the global SST and nino index routines need to read avgSurfaceSalinity instead of avgSurfaceTemperature. The trick though is knowing which variable to read. Maybe there could be a check if min(avgSurfaceTemperature(:,-1) < 10 then read avgSurfaceSalinity? But better to discuss in the mpas-analysis repo.

milenaveneziani · 2019-03-05T15:10:41Z

@vanroekel: I would do a mpas-a hack only locally (not on develop) and only for the simulation that is half-way through (hopefully it's only one (?)).

darincomeau · 2019-03-05T15:24:26Z

We have 8 simulations underway with the maint-1.2 branch started within the last week that would be impacted, but the furthest out is 30 years.

milenaveneziani · 2019-03-05T15:29:18Z

So, I would not change the code for the 8 simulations that are already under way, just have an extra version of mpas-a (we could tag it, for provenance) to analyze them.

Nothing is presently being stored in workArray(1,:) leading to all values in the var_array being shifted by one index. This is causing MPAS-Analysis failures. This results in avgSurfaceTemperature values being stored in avgSurfaceSalinity in output files. This commit shifts indices back one value so the ordering is as expected.

matthewhoffman · 2019-03-05T17:56:23Z

@vanroekel, I was able to force push to your fork after rebasing your branch to the commit that introduced the bug. The commit itself is unchanged, just its branch point.

mark-petersen · 2019-03-06T18:29:25Z

@jonbob This PR is number 3 on the list to get into E3SM master. I will merge this PR after the other two go in:

Update mpas-source, COMPASS only, no code changes E3SM-Project/E3SM#2779 Compass changes only
Time Varying Atmospheric Forcing #123 Time Varying Atmospheric Forcing (I'll make another E3SM PR)
This one.

Our philosophy here is to make smaller, more frequent commits into E3SM so we can more easily catch and fix bugs. I also don't want to merge into MPAS-Model:ocean/develop until it is next in line for E3SM.

jonbob · 2019-03-06T18:33:05Z

OK, I'll get started with 2779 today and we'll try to clear up the backlog

…elop Nothing is presently being stored in workArray(1,:) leading to all values in the var_array being shifted by one index. This is causing MPAS-Analysis failures. This results in avgSurfaceTemperature values being stored in avgSurfaceSalinity in output files. This commit shifts indices back one value so the ordering is as expected.

mark-petersen · 2019-03-21T15:22:42Z

Retested local merge with MPAS nightly regression suite, and on E3SM:

PASS PEM_Ln3.T62_oEC60to30v3wLI.GMPAS-DIB-IAF-ISMF.cori-knl_gnu RUN time=955
PASS PEM_Ln9.ne30_oECv3wLI.A_WCYCL1850-DIB-ISMF_CMIP6.cori-knl_gnu RUN time=665
PASS SMS_D.T62_oQU120_ais20.MPAS_LISIO_TEST.cori-knl_gnu RUN time=1114
PASS SMS_D_Ln9.T62_oQU120_ais20.MPAS_LISIO_TEST.cori-knl_intel RUN time=720

…elop Nothing is presently being stored in workArray(1,:) leading to all values in the var_array being shifted by one index. This is causing MPAS-Analysis failures. This results in avgSurfaceTemperature values being stored in avgSurfaceSalinity in output files. This commit shifts indices back one value so the ordering is as expected.

vanroekel requested review from mark-petersen, matthewhoffman and xylar March 5, 2019 05:24

milenaveneziani reviewed Mar 5, 2019

View reviewed changes

xylar approved these changes Mar 5, 2019

View reviewed changes

mark-petersen reviewed Mar 5, 2019

View reviewed changes

mark-petersen approved these changes Mar 5, 2019

View reviewed changes

xylar added bug Ocean labels Mar 5, 2019

matthewhoffman force-pushed the ocean/fixSurfaceAreaWeightedAM branch from 46726cb to 5a8f91c Compare March 5, 2019 17:53

matthewhoffman approved these changes Mar 6, 2019

View reviewed changes

mark-petersen merged commit 5a8f91c into MPAS-Dev:ocean/develop Mar 21, 2019

mark-petersen mentioned this pull request Mar 21, 2019

Fix indexing issue in ocean SurfaceAreaWeighted AM E3SM-Project/E3SM#2812

Merged

matthewhoffman added bug Ocean labels Mar 17, 2021

Fixes indexing issue in SurfaceAreaWeighted AM #173

Fixes indexing issue in SurfaceAreaWeighted AM #173

Uh oh!

Conversation

vanroekel commented Mar 5, 2019

Uh oh!

vanroekel commented Mar 5, 2019

Uh oh!

milenaveneziani Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

vanroekel Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

milenaveneziani Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

xylar left a comment

Choose a reason for hiding this comment

Uh oh!

xylar commented Mar 5, 2019

Uh oh!

milenaveneziani commented Mar 5, 2019

Uh oh!

mark-petersen Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

xylar Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

mark-petersen Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mark-petersen left a comment

Choose a reason for hiding this comment

Uh oh!

mark-petersen commented Mar 5, 2019

Uh oh!

xylar commented Mar 5, 2019

Uh oh!

xylar commented Mar 5, 2019

Uh oh!

xylar commented Mar 5, 2019

Uh oh!

milenaveneziani commented Mar 5, 2019

Uh oh!

mark-petersen commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vanroekel commented Mar 5, 2019

Uh oh!

milenaveneziani commented Mar 5, 2019

Uh oh!

darincomeau commented Mar 5, 2019

Uh oh!

milenaveneziani commented Mar 5, 2019

Uh oh!

matthewhoffman commented Mar 5, 2019

Uh oh!

mark-petersen commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonbob commented Mar 6, 2019

Uh oh!

mark-petersen commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mark-petersen Mar 5, 2019 •

edited

Loading

mark-petersen commented Mar 5, 2019 •

edited

Loading

mark-petersen commented Mar 6, 2019 •

edited

Loading

mark-petersen commented Mar 21, 2019 •

edited

Loading