Fix logic for adding psfc to aoflux_in

[billsacks]

Description of changes

Fix the logic for adding psfc to aoflux_in, particularly needed for cases using the exchange grid.

This is needed to avoid a segmentation fault in the call to flux_atmocn in SMS_Ld2.ne30pg3_t232.BMT1850.derecho_gnu.allactive-defaultio when running with aoflux_grid=xgrid. (This shows up when running with xgrid because the FB_fldchk can't be used in this situation - see details in my comment below.)

Specific notes

Contributors other than yourself, if any:

CMEPS Issues Fixed (include github issue #):

Are changes expected to change answers? No

Any User Interface Changes (namelist or namelist defaults changes)? No

Testing performed

SMS_Ld2.ne30pg3_t232.BMT1850.derecho_gnu.allactive-defaultio with aoflux_grid="xgrid", from cesm3_0_beta03

Also ran many other tests from the CESM prealpha and prebeta test suite.

[billsacks]

@DeniseWorthen or @uturuncoglu - I'd like at least a quick look from the UFS perspective to make sure this looks okay. In particular, please see a few lines below the change I made: lines 1658 - 1661 in this file, which has:

       if (trim(coupling_mode) == 'ufs.frac.aoflux') then
          call fldbun_getfldptr(fldbun_a, 'Sa_pslv', aoflux_in%psfc, xgrid=xgrid, rc=rc)
          if (chkerr(rc,__LINE__,u_FILE_u)) return
       end if

It seems to me that this block of code is unnecessary given the allocation on lines 1647-1648: I'm thinking that, in any case where you'd want to do the allocation on lines 1658-1661, it would already have done on line 1648, but I'm not sure that I'm understanding this right. It looks like the allocation on line 1648 was added more recently (by @mvertens ) than the one on lines 1658-1661, so I wonder if it was just an oversight at the time that the older one could now be removed. But again, I may be misunderstanding this and it may be that both are still needed.

The main reason I ask about this is because I could see there being problems if my new else block is executed - causing allocation of this variable - but then lines 1658 - 1661 are also executed.

[uturuncoglu]

Yes, it is strange. There is also one xgrid test under UFS and that one is also happy without this.

[uturuncoglu]

@billsacks maybe we could try to run couple of RTs under UFS WM to see what happens.

[billsacks]

Yes, it is strange. There is also one xgrid test under UFS and that one is also happy without this.

It seems like the logic for this field differs for CESM vs. UFS, so I wouldn't be surprised if things have been working fine for UFS without this change. I just want to make sure that I'm not breaking anything for UFS with the change!

maybe we could try to run couple of RTs under UFS WM to see what happens

That sounds great. But actually, I want to look more closely at this logic... I think my fix may not be right for the xgrid case... I need to run now but will look more closely at this later and then will reply.

[billsacks]

I think the fix I put in place is not the right fix. It looks like this psfc field isn't used in typical CESM runs, but the logic – both the pre-existing logic and the logic with my fix – is wrong in the CESM case where ocn_surface_flux_scheme = 2 and aoflux_grid = xgrid. I want to do a bit more analysis then will submit an updated fix.

[mvertens]

@billsacks - I think this is fine. It looks like this should not change anything for CESM - but is a fix for UFS.

[billsacks]

I have done some more analysis of this and changed the way I implemented the fix. The problem is that, for xgrid, you can't use a conditional like FB_fldchk(fldbun_a, 'Sa_pslv', rc=rc) as a guard around call fldbun_getfldptr(fldbun_a, 'Sa_pslv', aoflux_in%psfc, xgrid=xgrid, rc=rc), because with xgrid the fldbun_getfldptr subroutine is (somewhat confusingly, to me anyway) actually responsible for adding the field to fldbun_a. (This is in contrast to ogrid/agrid, where the fields are already present in fldbun_a before the call to fldbun_getfldptr.)

I have tried to do some careful looking at the existing code to figure out the right conditional for calling fldbun_getfldptr(fldbun_a, 'Sa_pslv', aoflux_in%psfc, xgrid=xgrid, rc=rc), and what I have – if ((trim(coupling_mode) == 'cesm') .or. & (trim(coupling_mode) == 'ufs.frac.aoflux' .and. trim(aoflux_code) == 'ccpp')) – seems probably correct. But @mvertens and @uturuncoglu – since it looks like the two of you put in place the earlier conditionals around this call, I'd like you both to look at this (Mariana with respect to CESM/NorESM and Ufuk with respect to UFS) and let me know that you agree with the new logic. I have two specific questions about this logic:

• Is coupling_mode == 'ufs.frac.aoflux' equivalent to the CPP token UFS_AOFLUX, and is coupling_mode == 'cesm' equivalent to CESMCOUPLED? If not, I may need to rework this logic: I came up with this logic based on the #ifdef blocks where psfc is needed, and these ifdefs use UFS_AOFLUX and CESMCOUPLED.
• Note that in the ufs-related conditional, psfc was previously being set based on compute_atm_dens or compute_atm_thbot together with ufs.frac.aoflux. I'm setting it whenever ufs.frac.aoflux and ccpp because it seems to be needed in the call (to flux_atmocn_ccpp) in those conditions.

[billsacks]

I have run extensive CESM testing that includes this fix – in the course of testing turning on xgrid by default, where I ran prealpha & prebeta testing on derecho. I'll comment on details in a separate PR that I'm opening shortly to change the default to xgrid, but the short summary is that all testing seems okay.

@uturuncoglu if you feel like it would be appropriate to run some UFS testing before this is merged, I'd welcome that.

[uturuncoglu]

@billsacks Sure. I'll update you when I have the results.

[billsacks]

Thank you @uturuncoglu !

[DeniseWorthen]

I ran a couple of tests w/ UFS (the agrid test and one of the tests that use the aoflux calculation) and they both passed. It seems fine for UFS, based on this limited testing.

[billsacks]

Thank you very much, @DeniseWorthen !

[billsacks]

@uturuncoglu - please let me know if we should wait for additional testing from you or if you feel that @DeniseWorthen 's testing is enough for UFS. Also please let me know if you have thoughts on the questions in my previous comment (no rush - I just wanted to make sure that those questions didn't get lost as the conversation turned to testing).

[uturuncoglu]

@billsacks No. I was doing the same thing with @DeniseWorthen. I think this is good to go.

[billsacks]

@mvertens confirmed for me that coupling_mode == 'cesm' is equivalent to the CPP token CESMCOUPLED (thanks, @mvertens !).

@uturuncoglu or @DeniseWorthen - I was looking a little more at options for coupling_mode and noticed that there is also an option 'ufs.nfrac.aoflux' in addition to 'ufs.frac.aoflux'. Do you know if my new conditional should also include 'ufs.nfrac.aoflux'? The old conditional (lines 1656-1659 in the left-hand side of the diff) was based on 'ufs.frac.aoflux', but I'm wondering if the 'ufs.nfrac.aoflux' case was being handled (maybe accidentally???) by lines 1647-1650 on the left-hand side of the diff. From a quick look through the code, I'm thinking that I probably should add 'ufs.nfrac.aoflux' here, but I haven't come to understand the difference between 'ufs.frac.aoflux' and 'ufs.nfrac.aoflux'. Or can you think of a simpler way to express this new conditional for the sake of UFS?

[DeniseWorthen]

@billsacks The only time UFS uses the aoflux calculations would be when the coupling_mode contains the sub-string 'aoflux', otherwise the fluxes to OCN are always calculated by the ATM.

I was actually a little surprised that your fix worked in the non-agrid test case I tried. It might be we could make your change a little cleaner, but I don't have the time right now to dig into it. Based on my two tests, it is OK for UFS and I can always come back w/ a fix if need be.

[billsacks]

The only time UFS uses the aoflux calculations would be when the coupling_mode contains the sub-string 'aoflux', otherwise the fluxes to OCN are always calculated by the ATM.

I was actually a little surprised that your fix worked in the non-agrid test case I tried. It might be we could make your change a little cleaner, but I don't have the time right now to dig into it. Based on my two tests, it is OK for UFS and I can always come back w/ a fix if need be.

Thanks @DeniseWorthen . I'm wondering, then, about changing my logic to be more general for the ufs case. Is it correct that med_phases_aofluxes_run should only be called when the coupling_mode contains the sub-string 'aoflux'? If so, I don't need to check for that explicitly in these routines (since we'll only be here if this string contains 'aoflux'), and I'm wondering if I should change my logic so that the psfc variable is set in this aoflux routine whenever we're running with ufs and using aoflux_code='ccpp' – i.e., if ((trim(coupling_mode) == 'cesm') .or. (coupling_mode(1:3) == 'ufs' .and. trim(aoflux_code) == 'ccpp')). @DeniseWorthen or @uturuncoglu does that seem reasonable to you?

[DeniseWorthen]

@billsacks That simplification of the logical condition also works. Thanks.

[billsacks]

I reran one test (SMS_Ld2.ne30pg3_t232.BLT1850.derecho_intel.allactive-defaultio) with my latest version. Given input from @DeniseWorthen and @uturuncoglu I'm going to go ahead and merge this with the latest version of the ufs logic.