Repository navigation
ctsl%rof_to_glc needs to be set before fldlist_add calls adding the Fgrg_rof? fields is done #103
Description
Activity
@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.
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.
@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.
I meant the issue not the PR.
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.
@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.Reacted by Erik Kluzek@slevis-lmwg I think this might be something that we should fix. I think it's simple to do...
@ekluzek am I understanding correctly that the "fix" here is the removal of the two lines shown at the top of the issue?
@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...
Reacted by Samuel Levis- added a commit that references this issue
on Nov 8, 2024 @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_roflshould be changed toFgrg_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: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.)
@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.
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.
Reacted by Erik KluzekReacted by Erik Kluzek@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.It looks like the fix for this came in with mosart1.1.08.
Reacted by Erik KluzekIn terms of process and to reduce risk in the future, is there a way to start labeling tags with known bugs (e.g. mosart1.1.02 in this case), so that questions get asked before such tags enter the coupled model?
Reacted by Erik KluzekIt's a good idea to reflect back and think about how to prevent this in the future. The thing is we usually only identify bugs after tags are created. So adding the label after it's used doesn't really help.
But maybe what we need is when we identify an important bug we figure out what CESM tags it was used in? Although even then we may not understand how big of a problem a particular bug is. I think that was the case here.
The other thing might be to do extra checking with the last tags that go into a beta tag. As usually the coupled testing is with the beta tag. The question is what to look for? We could make sure the beta testing has something that's a close replica of the coupled model testing and then maybe at least make sure namelists look right? But you can't really assess answer changes if answer changes are expected.
Which is part of the problem we have with answer changes. When we have answer changes it's hard to assess if there's a bug on top of those answer changes.
So I think it would be good to do more brainstorming on this. And would probably be good to look up the history on when we identified it as a problem and if we did enough checking to make sure we assessed how big of a problem it was.
That is something we should do with an answer changing bug -- assess how big of a problem it might be. That's the one clear idea on something to check on.
Other thoughts here? Should we meet to discuss? I'm thinking we should...
Referring to the part of this that ended up being an important science bug for coupled simulations:
@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.
....@briandobbins @billsacks and @slevis-lmwg and I met and we discussed this. We'll bring that discussion up in the CSEG meeting as well. We thought of some ideas to help mitigate this type of problem for the future:
- Adding a line item to CSEG to discuss important bugs that might affect science and especially coupled simulations
- When an important science bug is found, add it to planned tags as something to fix that HAS to go into the next beta tag
- Do a tag with just the one fix in
- Remember when testing answer changes, and refactoring is done – to test that the refactoring is bit-for-bit on top of the answer changes
- Remember to ask the question when something changes answers – will this affect the science and how extensively? Will it affect coupled simulations? And do some simulations to verify it’s small…
- One way to assess coupled impact is to look at the cpl history fields in your testing -- if those change it WILL affect the coupled model
@ekluzek in the above list it may be relevant to add something I brought up in our meeting:
The particular bug manifested as baseline DIFFs in both mosart and cpl history. DIFFs in cpl history seem like a logical alarm trigger when asking whether a bug will affect the coupled model.
Reacted by Erik Kluzek@ekluzek in the above list it may be relevant to add something I brought up in our meeting:
The particular bug manifested as baseline DIFFs in both mosart and cpl history. DIFFs in cpl history seem like a logical alarm trigger when asking whether a bug will affect the coupled model.
Good point @slevis-lmwg. I added to the list above.
#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).
So the setting of ctl%rof_from_glc should be set before the above and the above changed to...
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.