ESCOMP / MOSART

Model for Scale Adaptive River Transport, Mosart, part of the Community Earth System Model
http://www.cesm.ucar.edu/
Other
8 stars 27 forks source link

Remove non-working aspects of the FLOOD option #80

Closed ekluzek closed 8 months ago

ekluzek commented 8 months ago

@mvertens found this issue and does work on this in #74.

The FLOOD option currently can NOT be turned on because of the following logic in RtmMod.F90 where it aborts if it's turned on. But, more than that @mvertens found that it does NOT work if it's turned on. So it's a dead code option.

    !-------------------------------------------------------
    ! Initialize mosart flood - rtmCTL%fthresh and evel
    !-------------------------------------------------------

    if (do_rtmflood) then
       write(iulog,*) subname,' Flood not validated in this version, abort'
       call shr_sys_abort(subname//' Flood feature unavailable')
       call RtmFloodInit (frivinp_rtm, rtmCTL%begr, rtmCTL%endr, rtmCTL%fthresh, evel)
    else
       effvel(:) = effvel0  ! downstream velocity (m/s)
       rtmCTL%fthresh(:) = abs(spval)
       do nt = 1,nt_rtm
          do nr = rtmCTL%begr,rtmCTL%endr
             evel(nr,nt) = effvel(nt)
          enddo
       enddo
    end if

We definitely want to remove non-working code, but in further discussion with @swensosc and @slevis-lmwg there is a desire to have the FLOOD capability to still be part of the model, but reworked. I'll file that as a follow on issue to this one.

mvertens commented 8 months ago

@ekluzek - I must admit I'm totally confused about what the current plan is. Is this holding up the PR #74? Since this feature wasn't even working before and the model crashed if you tried to turn on flood - it seems to me that this new capability can be put in subsequent to PR #74 being accepted.

ekluzek commented 8 months ago

@mvertens I gave an update here about our plans -- https://github.com/ESCOMP/RTM/pull/43#issuecomment-1897894653. @slevis-lmwg and I did meet Friday and we decided exactly what to do, and we will make the tags when we meet again on Monday afternoon. I suppose I should have given an update after Friday since we didn't actually make the tags then.

In our meeting with @swensosc we do need to get it in a state that he can use flooding. He has been using it, although I think even he would admit it's in a non-ideal manner, as he has to add code modes to get it to work. But, he has been using it actively and it's important that he can do this and not lose that functionality now. #81 will be to put this in properly and add testing for it and that will be a future capability that will be done later. So that won't affect anything that is being worked on now...

mvertens commented 8 months ago

@ekluzek - thanks for the clarification.

mvertens commented 8 months ago

@ekluzek @slevis-lmwg @swensosc - I am wondering why this will not go in after #76 - which is a major cleanup of the code and also provides new needed functionality. It seems that this would be easier to incorporate into the refactored code. Otherwise - this will again need to be redone for the refactored code.

mvertens commented 8 months ago

On the other hand - if you all feel that this really needs to go in now, I can bring in these changes into my refactored branch. Just let me know what makes the most sense.

wwieder commented 8 months ago

Thanks for all the discussion on this. I'm a little unclear on the suggested order of PRs to resolve the following issues, but is this still the plan?