-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove non-working aspects of the FLOOD option #80
Comments
@mvertens I gave an update here about our plans -- ESCOMP/RTM#43 (comment). @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... |
@ekluzek - thanks for the clarification. |
@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. |
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. |
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?
|
@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.
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.
The text was updated successfully, but these errors were encountered: