ctsl%rof_to_glc needs to be set before fldlist_add calls adding the Fgrg_rof? fields is done

[ekluzek]

#94 introduces a control option ctl%rof_from_glc. However, this flag is being set after the fldlist_add calls (which should be inside an if statement for that control option).

    call fldlist_add(fldsToRof_num, fldsToRof, 'Fgrg_rofl') ! liq runoff from glc
    call fldlist_add(fldsToRof_num, fldsToRof, 'Fgrg_rofi') ! ice runoff from glc

So the setting of ctl%rof_from_glc should be set before the above and the above changed to...

    if ( ctl%rof_from_glc ) then
       call fldlist_add(fldsToRof_num, fldsToRof, 'Fgrg_rofl') ! liq runoff from glc
       call fldlist_add(fldsToRof_num, fldsToRof, 'Fgrg_rofi') ! ice runoff from glc
    end if

Doing this will enable MOSART to work with current and previous versions of CISM, right now it will die when it gets to the fldlist_add call as Fgrg_rofl is not being sent from CISM.

[jedwards4b]

@ekluzek Is there a compelling reason to make this compatible with older versions of cism? I don't see a strong argument for backward compatibility.

[ekluzek]

It would've helped with the transition. But moving forward there isn't a compelling need for it.

But, my point is that the logic should be fixed or removed. If we are good with just removing it we could go that route as well and change the name of this.

[mvertens]

@ekluzek - I removed ctl%glc2rof this morning. At this point mosart always sends the glc runoff if the the coupler advertises that it will be sent. However, the field is not actually filled in with non-zero values unless glc -> rof is active and the field actually exists in the glc export:

! custom merge for glc->rof
  ! glc->rof is mapped in med_phases_post_glc
  do ns = 1,is_local%wrap%num_icesheets
    if (is_local%wrap%med_coupling_active(compglc(ns),comprof)) then
      do nf = 1,size(fldnames_fr_glc)
        if ( fldbun_fldchk(is_local%wrap%FBImp(compglc(ns),comprof), fldnames_fr_glc(nf), rc=rc) .and. &
             fldbun_fldchk(is_local%wrap%FBExp(comprof), fldnames_fr_glc(nf), rc=rc) ) then
          call fldbun_getdata1d(is_local%wrap%FBImp(compglc(ns),comprof), &
               trim(fldnames_fr_glc(nf)), dataptr_in, rc)
          if (chkerr(rc,__LINE__,u_FILE_u)) return
          call fldbun_getdata1d(is_local%wrap%FBExp(comprof), &
               trim(fldnames_fr_glc(nf)), dataptr_out , rc)
          if (chkerr(rc,__LINE__,u_FILE_u)) return
          ! Determine export data
          if (ns == 1) then
            dataptr_out(:) = dataptr_in(:)
          else
            dataptr_out(:) = dataptr_out(:) + dataptr_in(:)
          end if
        end if
      end do
    end if
  end do

Since this logic has already been removed I'm not sure what the status of this PR should be.

[mvertens]

I meant the issue not the PR.

[ekluzek]

Hmmm. I still see it in what I just updated to. And I see it in the PR as it exists now...

git grep rof_from_glc | more
src/cpl/nuopc/rof_import_export.F90:      ctl%rof_from_glc = .true.
src/cpl/nuopc/rof_import_export.F90:      ctl%rof_from_glc = .false.
src/cpl/nuopc/rof_import_export.F90:      write(iulog,'(A,l1)') trim(subname) //' rof from glc is ',ctl%rof_from_glc
src/cpl/nuopc/rof_import_export.F90:    if (ctl%rof_from_glc) then
src/riverroute/mosart_control_type.F90:     logical :: rof_from_glc                         ! if true, will receive liq and ice runoff from glc
src/riverroute/mosart_driver.F90:      if (ctl%rof_from_glc) then

Is there perhaps some commits that haven't been pushed to your fork? Or outstanding changes that didn't get committed and need to be and pushed?

Assuming that's the case -- we'll just close this one as resolved in #94.

[mvertens]

@ekluzek - Sorry for the confusion. I should not write when its so late.
You are right - this is still there. The key point is that I put the ctl%rof_from_glc in order to be backwards compatible with older versions of cmeps.
The logic is not in the advertise - but in the realize. If cmeps does not advertise these fields then there is no connection and the fields won't be realized - hence the logic in rof_import_export: realize_fields:

    if (fldchk(importState, 'Fgrg_rofl') .and. fldchk(importState, 'Fgrg_rofl')) then
      ctl%rof_from_glc = .true.
    else
      ctl%rof_from_glc = .false.
    end if

However, i just realized that in older version of cmeps Fgrg_rofl and Fgrg_rofl are not defined in fd_cesm.yaml - so the backwards compatibility will be broken. I think at this point the code is working and I can try to address this issue later.
That said - thanks for pointing this out.

[ekluzek]

@slevis-lmwg I think this might be something that we should fix. I think it's simple to do...

[slevis-lmwg]

@ekluzek am I understanding correctly that the "fix" here is the removal of the two lines shown at the top of the issue?

[ekluzek]

@slevis-lmwg no what I suggest above was to add an if statement around them. But, we need to reexamine this and make sure that's correct. It might be good to have a test case for it as well. But, I'm not sure we need to spend too much time on this either...

[billsacks]

@slevis-lmwg and I were talking about this in the context of his failing baseline comparisons in ESCOMP/CTSM#2838.

As @slevis-lmwg found with his great sleuthing to track down the issues in that PR (ESCOMP/CTSM#2838 (comment)), it looks like the current logic isn't right: now this rof_from_glc logical is being checked (in advertise_fields) before it's set (in realize_fields), leading it to be false in advertise_fields when it should be true - and possibly unpredictable / unrepeatable behavior.

@slevis-lmwg has found that removing the conditional around the fldlist_add in advertise_fields – i.e., the conditional that was added here 692d183 – allows tests to pass both with CISM present and with SGLC.

@mvertens - Before moving ahead with that solution, I want to confirm that this seems like the right thing to do, particularly thinking about the case with SGLC. (I think we aren't so much concerned with backwards compatibility at this point.) I have two questions:

(1) With this proposed change - which I think reverts things to basically how you originally implemented it, but I'm not positive - we'll have:

       call fldlist_add(fldsToRof_num, fldsToRof, 'Fgrg_rofl') ! liq runoff from glc
       call fldlist_add(fldsToRof_num, fldsToRof, 'Fgrg_rofi') ! ice runoff from glc

in advertise_fields, and then

    if (fldchk(importState, 'Fgrg_rofl') .and. fldchk(importState, 'Fgrg_rofl')) then
      ctl%rof_from_glc = .true.
    else
      ctl%rof_from_glc = .false.
    end if

in realize_fields. (Side note: I think that second Fgrg_rofl should be changed to Fgrg_rofi, but it shouldn't matter in practice.) My sense is that, when running with SGLC: MOSART adds these fields in the advertise, but I think CMEPS doesn't add them to its field list, because I think - but am not sure - that num_icesheets will be 0 here:

https://github.com/escomp/CMEPS/blob/aa7f5c109216309889e6c51320c392cdeb1e64ee/mediator/esmFldsExchange_cesm_mod.F90#L3189

So I think that, in MOSART's calls to fldlist_realize, these Fgrg things end up being removed from the state. I haven't looked or tested carefully to confirm, but does this sound right?

(2) Is this the correct, intended behavior: that rof_from_glc will be False when running with SGLC? (I'm guessing yes, but want to confirm.)

[mvertens]

@billsacks - I think your logic is correct. The mediator always advertises everything - but if the component does not advertise it then no connection is made and in the realize phase these fields will not be added to the export state. In addition - as you mentioned - for a stub glc the mediator sets num_icesheets to 0 and in this case will not advertise these fields either.

[billsacks]

Thank you, @mvertens!

@slevis-lmwg and @ekluzek - my takeaway from this is that @slevis-lmwg 's change that he tested yesterday - reverting the change he made in mosart1.1.03 - seems like the right thing to do. But I'll admit that I haven't come to understand the full motivation for that change in the first place, so you two should verify that that feels correct to you, too.

[slevis-lmwg]

@billsacks thank you for working with me on this and @mvertens thanks for the validation of Bill's understanding.

Motivation behind the if-statement (@ekluzek will need to confirm):
I understood it as backwards compatibility, plus explicit understanding of which lines are needed when. Since the code works correctly without the if-statement and breaks with it, I will replace it with a comment.