My apologies for this. When Nils was originally testing this AM, the dimension maxRegionsInGroup was only in the MHT, so we put in a 3. We should have put a comment with a warning.

@vanroekel could you test this on your new run on COMPY with the new MOC mask file? It should work.

@mark-petersen sure, I will test this. For some reason I can't get on compy right now but will test this as soon as I can get back on.

hmm, I should have caught this. thanks @mark-petersen.

No problem. It was a silly past reviewing oversight on my part.

E3SM with this change passed:

./create_test SMS_Ld5.T62_oEC60to30v3.GMPAS-IAF.cori-knl_intel

including a call to the moc analysis member.

okay I'm back on compy. testing now.

with the change on compy the segfault moves it is now here (bold line below)

         !!!! TRANSECT CALCULATION
         sumTransport = 0.0_RKIND
         do iTransect = 1,transectsInAddGroup
            currentTransect = transectsInGroup(iTransect, transectGroupNumber)
            do iEdge = 1,nEdgesSolve
               c1 = cellsOnEdge(1,iEdge)
               c2 = cellsOnEdge(2,iEdge)
               do k = 1, maxLevelEdgeTop(iEdge)
                  sumTransport(k,iTransect) = sumTransport(k,iTransect) + &
                     transectEdgeMaskSigns(currentTransect,iEdge) &
                     * transectEdgeMasks(currentTransect, iEdge) &
                     *** normalVelocity(k,iEdge)*dvEdge(iEdge) &**
                     * 0.5_RKIND*(layerThickness(k,c1) + layerThickness(k,c2))
               end do
            end do
            do k = 2, nVertLevels
               mocStreamValLatAndDepthRegionLocal(1, k, iTransect) = &
                  mocStreamValLatAndDepthRegionLocal(1, k - 1, iTransect) &
                  + sumTransport(k - 1, iTransect)
            end do
         end do

sorry, the bold didn't work, it is line 514, of the moc_streamfunction.F file

OK. I'll see if I can reproduce in stand-alone. @milenaveneziani I got the namelist and streams from compy.

@vanroekel, this is maybe the better way to show one or more relevant lines of code like you were trying to do:
https://github.com/mark-petersen/MPAS-Model/blob/ocean/fix_moc_error/src/core_ocean/analysis_members/mpas_ocn_moc_streamfunction.F#L514
Let me know if you don't know how to do this.

@mark-petersen: look at line 382. There is another dimension 3 hard wired.

@xylar thanks for the suggestion, that is much better. I was able to figured out how to do that now.

@mark-petersen and @milenaveneziani I just tested with changing L382 and have a segfault in the same place.

OK, I'll test in stand-alone to debug.

OK, there were two more problems:

1. The seg fault mentioned in Fix analysis member MOC region dimension #374 (comment). When we use:
   config_am_mocstreamfunction_normal_velocity_value = 'normalTransportVelocity'
   it tries to access from the 'state' var_struct rather than 'diagnostic' here:
   496 call mpas_pool_get_array(statePool, normalVelocityArrayName, normalVelocity, timeLevel)
   I propose that we always use 'normalTransportVelocity' here for all simulations, and remove this flag.

2. The StrLen in the new MOC mask file is 1024 rather than 64. This causes all string arrays to be read in as blank. In particular, transectGroupNames and regionGroupNames, so a match is never found, and regionGroupNumber and transectGroupNumber remain zero and can cause a seg fault. It appears StrLen=64 is hardwired within MPAS framework, though I couldn't find it. The simple solution here is to just use StrLen=64. On the current mask file:
   ncks -d StrLen,0,63 in.nc out.nc
   (Thanks @milenaveneziani for pointing this out)

Thanks @mark-petersen for summarizing these. Are you going to try a standalone test with the changes you propose?

I realize we don't have time for this now but can we make an issue about trying to fix the hard-coding of StrLen instead of shortening strings? It would be really easy to end up in situations where 64 characters isn't enough for a unique name and chopping strings is obliterating a bunch of provenance.

A problem will immediately arise, which is that StrLen is a single dimension for all of MPAS. I'm not sure if it's defined in framework, or if it gets it from the first file it reads in. Regardless, we would have to change the string length on ALL files read into any MPAS simulation, and break backwards compatibility with strings in older files.

A second option would be to change the framework to test the string length on input and alter 64 character strings. Either way, I don't think 64 character strings is limiting enough for that much trouble.

@vanroekel I fixed both problems I described above in #374 (comment) and also made the AM more fail safe. You should reduce the mask file to 64 character length with
ncks -d StrLen,0,63 in.nc out.nc
But if we use a 1024 character one by mistake, it will produce write a warning and use the first region and transect, but still run with this file.

I tested in stand-alone with both mask files. @vanroekel please test in your run.

All the AMs deserve more attention, so we can use multiple region groups and transect groups. It's on my list, but this will have to do for today.

I really disagree about supporting reading the first 64 char of a longer string. I somewhat disagree about 64 char being sufficient but realistically I won't be able to change this...

@mark-petersen my testing found another issue here
https://github.com/MPAS-Dev/MPAS-Model/blob/ocean/develop/src/core_ocean/analysis_members/Registry_moc_streamfunction.xml#L72-L74

This array is hardcoded to 3 regions and is causing a segfault in my testing with the new transect file where there are 4 regions. -- The segfault is here
https://github.com/MPAS-Dev/MPAS-Model/blob/ocean/develop/src/core_ocean/analysis_members/mpas_ocn_moc_streamfunction.F#L239

Is there a reason for that array (minMaxLatRegion) to be in the pool and not an allocatable in the AM like the other minMax arrays?

I changed the dimension to nRegions.

Thanks. With the last changes, I was able to run on compy with the new transect. I'll try post some pictures from the two runs tomorrow.

Here are some figures of the AMOC from months 1-6 of a G-case
The new transect file

Here is the original

Here is the difference (new transect - old transect)

They are qualitatively similar, with the biggest differences around 35N and at depth. I'm not sure I'd read too much into such a short simulation at depth anyway.

cc'ing @maltrud for his input on the differences as well.

Thanks @vanroekel !
Interesting pattern between 35 and 40..
Also interesting that the difference at 26N (where we have been focusing in terms of difference old-new in the offline MOC calculation) is not that striking instead.

@milenaveneziani looks like the diffs in the NH could be attributed to differences in regions between the files. Here is a picture (red is old file, black is new). The old did not include the med or far north seas.

yes, that sounds reasonable.
Should we approve of these new transects, then?

I should also check the MOC results for the other 3 transects, to see if the numbers make sense.

@milenaveneziani, @mark-petersen and @vanroekel, what is the status here? What needs to happen before this can be merged?

in #374 (comment), @mark-petersen wrote:

The StrLen in the new MOC mask file is 1024 rather than 64. This causes all string arrays to be read in as blank. In particular, transectGroupNames and regionGroupNames...

Two years ago, I made this change in the MPAS Mask creator:
MPAS-Dev/MPAS-Tools#185
No one pointed out any issues this might have at the time. No one has reported any problems using masks created with the tool until now. So it seems like the problem with longer strings may be specific to the MOC analysis member rather than MPAS-Ocean more broadly. That, or maybe the strings in mask files are not being used in other contexts?

Does this change to the mask creator need to be reverted? As I said, I see the 64 character limit as a big problem for provenance but @mark-petersen's suggested NCO trick (ncks -d StrLen,0,63 in.nc out.nc) doesn't seem like a desirable step to have to add to every workflow that uses masks.

I can easily incorporate a step that truncates strings to 64 characters in the tool that creates the southern MOC transects if the problem is specific to the MOC analysis member. I'll hold off on doing so, however, until it's clear whether or not we want to change this in the mask creator itself instead.

We are merging in all PRs on the E3SM V2 PR list (made in June) first. Then we can merge all these more recent ones in, probably in two weeks.

I can easily incorporate a step that truncates strings to 64 characters in the tool that creates the southern MOC transects if the problem is specific to the MOC analysis member

@xylar, we would have to check to make sure, but I predict this is a problem with at least the MOC and MHT analysis members. We would also have to check the relatively recent changes made to surfaceAverages and layerWeigthedAverages to do regional averages.
Mind you that this is the first time that we tried to use the new mask files with the analysis members, which is why these problems haven't come up before.