Bugfix for failing C compset DEBUG tests

[mnlevy1981]

The fldlist argument of seq_map_map() (called from prep_ocn_calc_r2x_ox) varies depending on whether the ocean is the only active component or not. I think this is the right place for the pull request, though maybe it needs to come through ESMCI?

Test suite: two tests from aux_pop (one that had been failing previously) Test baseline: none (no baseline available since answer-changing update to make WW default) Test namelist changes: none Test status: bit for bit

Fixes -- I don't believe an issue ticket was filed for this, see "code review" section for details of problem

User interface changes: None

Code review: I'd appreciate it if someone familiar with this part of the code would look things over; with DEBUG=TRUE I was getting an error in prep_ocn_mod.F90. Two changes had been made in CIME that affected this file since the last successful test: water isotopes were added and then the runoff file was split into liquid and ice runoff (needed for estuary box model in POP). With just two changes to consider it was pretty straight-forward to track down the problem, but I don't know if my fix is consistent with the coding practices of the module.

When water isotopes were added to the coupler, the runoff -> ocean fields changed -- this is fine in coupled and ocean / ice cases, but the new isotope fields should not be used in ocean-only compsets. To get around this, I introduced a new module variable (lwiso_runoff) in prep_ocn_mod.F90 that is true if index_x2o_Foxx_rofl_16O is non-zero; if lwiso_runoff is true then seq_map_map is called with a full list of fields (i.e. all the water isotopes) otherwise it is just called with Forr_rofl and Forr_rofi.

[jtruesdal]

It just so happens that Cecile's development run 121 is dying in the exact same line when running with debug. Her run is coupled though, is it possible that this could also be a problem in coupled runs? What would I look at to see if isotopes are turned on Cecile's run?

jt

On Mon, Oct 17, 2016 at 2:56 PM, Bill Sacks notifications@github.com wrote:

@billsacks requested changes on this pull request.

This basically looks good to me. I have some minor comments about things you could do to clean it up a bit (in my opinion). You may also want to see prep_rof_mod changes in ESMCI#681 https://github.com/ESMCI/cime/pull/681

I think this PR should go in ESMCI: it feels like we're putting all PRs

there now.

In driver_cpl/driver/prep_ocn_mod.F90 https://github.com/CESM-Development/cime/pull/478#pullrequestreview-4555652 :

@@ -650,6 +653,7 @@ subroutine prep_ocn_merge( flux_epbalfact, a2x_o, i2x_o, r2x_o, w2x_o, g2x_o, xa index_x2o_Faxa_snow_16O = mct_aVect_indexRA(x2o_o,'Faxa_snow_16O' , perrWith='quiet') index_x2o_Faxa_prec_16O = mct_aVect_indexRA(x2o_o,'Faxa_prec_16O' , perrWith='quiet') index_x2o_Foxx_rofl_16O = mct_aVect_indexRA(x2o_o,'Foxx_rofl_16O' , perrWith='quiet')

• lwiso_runoff = ( index_x2o_Foxx_rofl_16O /= 0 )

It seems like the way you've done this works, but I'd prefer to see the setting of lwiso_runoff done in prep_ocn_init rather than prep_ocn_merge: Then I'd be more confident that this is definitely set before it is referenced.

Then, optionally, you could use the already-set lwiso_runoff in prep_ocn_merge to determine whether to try to get the various 16O and 18O

fields.

In driver_cpl/driver/prep_ocn_mod.F90 https://github.com/CESM-Development/cime/pull/478#pullrequestreview-4555652 :

@@ -988,7 +992,7 @@ subroutine prep_ocn_merge( flux_epbalfact, a2x_o, i2x_o, r2x_o, w2x_o, g2x_o, xa g2x_o%rAttr(index_g2x_Fogg_rofi , n)) * flux_epbalfact

• if ( index_x2o_Foxx_rofl_16O /= 0 ) then
• if ( lwiso_runoff ) then

I know you have basically made lwiso_runoff a synonym for index_x2o_Foxx_rofl_16O, but this code would feel better (more symmetrical) to me if you either:

(a) kept this line as it was before

or

(b) determined the value of lwiso_runoff based on index_x2o_Foxx_rofl_16O (as you currently do), then made the use of all isotope fields

dependent on lwiso_runoff

In driver_cpl/driver/prep_ocn_mod.F90 https://github.com/CESM-Development/cime/pull/478#pullrequestreview-4555652 :

 !---------------------------------------------------------------

 call t_drvstartf (trim(timer),barrier=mpicom_CPLID)

• if (lwiso_runoff) then
• fldlistl='Forr_rofl:Forr_rofl_16O:Forr_rofl_18O:Forr_rofl_HDO'

This hard-coding of field lists feels very fragile to me. But I guess that's not your fault. I'm fine with the change you made here, given that this list was already hard-coded.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/CESM-Development/cime/pull/478#pullrequestreview-4555652, or mute the thread https://github.com/notifications/unsubscribe-auth/AI223-WLrQM-fhppjSq-Sxkhyy1SqGq4ks5q0-D-gaJpZM4KZE9W .

[mnlevy1981]

@billsacks -- thanks for the comments! I'll reply in line where appropriate. Jim also said to put this in ESMCI so I'll close this request and point to it from an ESMCI issue ticket once said ticket exists.

@jtruesdal I haven't done a good job tracking down what actually triggers whether we map these isotopes over or not. I had assumed that it was an rtm vs drof issue, but in typing that response I realized I had forgotten about MOSART. Also, it doesn't make sense to me that a C compset and a G compset would use different runoff components... I'll let you know if I find anything more conclusive while trying to follow Bill's suggestion to set lwiso_runoff in the initialization routine.

[jtruesdal]

I've been doing a little research as well.

It looks to me like these fields are triggered via the drv_in namelist variable flds_wiso. seq_flds_mod.F90 is where this is read and the initial isotope fields are enabled.

In Cecile's experiment this is set to false so Mikes suggested fix is required for her code to run under debug.

Still hand building the fieldlist is a bit of a hack. All of the other fields are accumulated in specific character arrays as the fields are enabled in seq_flds_mod and the seq_map_map calls simply point to this character array. Something like

fldlist=seq_flds_a2x_fluxes

I think seq_flds_mod needs to be changed to accumulate the liq and ice runoff fields in separate character arrays (Maybe seq_fld_r2x_liq_fluxes and seq_fld_r2x_ice_fluxes?). At present they are both being gathered into seq_flds_r2x_fluxes.

Once this is done I think you can do away with hand building separate lists for the liquid and ice mapping and use the same construct as above:

fldlist=seq_flds_r2x_liq_fluxes

and

fldlist=seq_flds_r2x_ice_fluxes

You won't need a new module level variable either for the prep_ocn_calc_r2x_ox routine. You can use a logical if you like but it would only be needed in prep_ocn_merge.

jt

On Mon, Oct 17, 2016 at 11:24 PM, Michael Levy notifications@github.com wrote:

@mnlevy1981 commented on this pull request.

In driver_cpl/driver/prep_ocn_mod.F90 https://github.com/CESM-Development/cime/pull/478:

 !---------------------------------------------------------------

 call t_drvstartf (trim(timer),barrier=mpicom_CPLID)

• if (lwiso_runoff) then
• fldlistl='Forr_rofl:Forr_rofl_16O:Forr_rofl_18O:Forr_rofl_HDO'

Yeah, I don't know how to get around that. I thought about getting fancy and setting

fldlistl='Forr_rofl'

and then appending the other three fields if lwiso_runoff is true, but that's still hard-coding in the field names. (Though it would be a preferable construct if some future option that is independent of the water isotopes could also impact what runoff fields are mapped.)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CESM-Development/cime/pull/478, or mute the thread https://github.com/notifications/unsubscribe-auth/AI22318nZz4JTFkSO8w6cgbiwESCHqJkks5q1DvqgaJpZM4KZE9W .

[billsacks]

I like @jtruesdal 's suggestion. If you want to see examples of something similar in seq_flds_mod, you can see the various _to_glc, _from_glc, _to_lnd and _from_lnd variables: Note that fields get added to these in addition to the "standard" field lists (seq_flds_r2x_fluxes in your case, I think) - so the standard lists can be referenced when needed, and the special-purpose lists can be referenced when needed.

And it looks to me like, once that's done, you can actually remove all mods from prep_ocn_merge (unless I'm missing something) - i.e., I don't think you'll need the lwiso_runoff variable any more. If I understand the code correctly, that would go back to separately checking the presence of each isotope type (16O, 18O, HDO) during the merge.

[mnlevy1981]

@jtruesdal Thanks for digging in to this! Your approach definitely sounds superior, I'm going to close this pull request, delete my branch, and start from scratch with your implementation. And @billsacks I agree, this would remove the need for lwiso_runoff (which seemed like a pretty kludgy implementation even when I was first putting it in)