search

CESM-Development / cime

Common Infrastructure for Modeling the Earth
Other
16 stars 13 forks source link

fix shr_flux_atmocn_diurnal restart problem related to cold start initialization logic #422

Closed apcraig closed 8 years ago

apcraig commented 8 years ago

This corrects an error in the initialization logic in shr_flux_atmocn_diurnal. A new argument, cold_start, has been added to the interface to indicate to the subroutine whether the interface should reset the flux fields. This should only happen on initial runs without a restart file. The original logic was not robust.

Test suite: ERP_Ln9.ne30_ne30.FC55CLUBB.yellowstone_intel.cam-outfrq9s Test baseline: cesm1_5_alpha06g Test namelist changes: none Test status: bit for bit but fixes exact restart bug

Fixes: [CIME Github issue # ] atmocn flux diurnal restart bug

User interface changes?: none

Code review: none

jedwards4b commented 8 years ago

Thanks for tracking this down. I have reviewed the changes that you made and they look fine. However the pull request has changes in it that I am sure that you didn't make, I think that you need to try creating the pull request again from the head of master so that we don't bring in the unwanted changes in allactive/config_compsets.xml and allactive/testlist_allactive.xml.

billsacks commented 8 years ago

Your changes seem like an improvement. However, I'm wondering if most of the initialization in the "cold_start" conditional in shr_flux_mod is really needed at all. From spot-checking, it appears that at least some of this initialization is already done in seq_flux_init_mct. This appears not to be the case for tSkin_day and tSkin_night, but ideally (in my mind) their initialization would be moved outside of here as well.

I suggest this because I find it confusing when code called from the run loop has initialization logic like this. It gets harder to reason about how things are initialized in various cases, what needs to be done upon restart, etc. I find it easier to understand when initialization is done in its own place, called from the initialization sequence. Since there already appears to be a place to initialize (most of) these variables (seq_flux_init_mct), I'm wondering if it would be possible to just use that routine rather than having this cold_start logic?

apcraig commented 8 years ago

Jim, the changes in the allactive files are the diffs from cime4.5.2 and cime4.5.3 that I pulled in manually. I assume when you merge, those diffs will have no effect.

Bill, I think you're right. It would be better to fix this properly. I have tried to get more information on a number of occasions about what the atmocn diurnal subroutine should do so I could help review it and improve it, but that really went nowhere. To be honest, I'm not convinced that anyone that has been working on this subroutine really understands it. I know I don't. I don't think it's my call to unilaterally work through the logic to try to understand it and fix it. This should be a collaborative process that all contributors should agree needs to happen. Maybe that has happened without me and that's fine. But I haven't been a part of any process like that. I think it's worthwhile to have a process like that for this part of the code, and I have tried to encourage something like that a couple times, but so far, it hasn't happened. I'm happy to contribute to a code review and any refactoring needed moving forward.

billsacks commented 8 years ago

@apcraig : Okay, that makes sense. I had assumed that you were more involved with this code earlier because your name was attached to the comment in the old code:

!     tcraig, dec 2013, this should only occur on a cold startup.

but it sounds like that was an incorrect assumption.

I'm okay deferring this cleanup, but I agree with you that it a more careful review / cleanup should really happen at some point.

Anyway: Nice work tracking down this problem!