Fix Leap Year Flag Issue #7199
Fix Leap Year Flag Issue #7199
Conversation
|
Fixes #7173. Let me know if you'd like me to undo the clang-format changes. |
There was a problem hiding this comment.
Pulled latest develop and built locally. Ran the new unit test and it passes and tests exactly what this fixes. This is a nice compact fix and test. Thanks @jasondegraw. It should merge shortly, I just want to go back and make sure there weren't other comments on it.
| LocalLeapYearAdd = 1; | ||
| } else { | ||
| env.IsLeapYear = false; // explicit set, this might be unwise | ||
| } |
There was a problem hiding this comment.
OK, so the weather file leap year support is only checked if the current year is actually a leap year. Makes sense.
There was a problem hiding this comment.
@jasondegraw hey, should we have put a warning in here for if the runperiod is a leap year but the weather file doesn't support it? If so is this the right spot for it? We'll add it within a different branch if so. (Or you could add it to the namespace change branch on the next merge up)
There was a problem hiding this comment.
@Myoldmopar I'm not sure it is the right spot to add a warning. It would be good to have a warning, but my preference would be to wait until weather handling gets refactored. If we put in an issue on the lack of warning it'll be easier on whoever does the refactor. If we add the warning now, it'll be one more thing that someone will have to find. Previous behavior was to be silent, so I think it'd be alright to leave it alone for a bit longer, but if you'd rather add the warning that's OK too.
There was a problem hiding this comment.
Given how late in the game we are right now, I support the idea of doing nothing but opening up a ticket. Thanks @jasondegraw
| "RP3, !- Name", | ||
| "1, !- Begin Month", | ||
| "1, !- Begin Day of Month", | ||
| "2019, !- Begin Year", |
There was a problem hiding this comment.
So this unit test should not be a leap year, but I'm guessing we'll set the weather file up as leap year capable.
| WeatherManager::TotRunPers = 1; | ||
| WeatherManager::TotRunDesPers = 0; | ||
|
|
||
| WeatherManager::WFAllowsLeapYears = true; // This was hitting a bad bit of logic |
There was a problem hiding this comment.
So the run period should never even check the leap year capability of the weather file, meaning we won't have a chance to check this flag. OK.
| WeatherManager::WFAllowsLeapYears = true; // This was hitting a bad bit of logic | ||
| WeatherManager::SetupEnvironmentTypes(); | ||
|
|
||
| EXPECT_FALSE(WeatherManager::Environment[0].IsLeapYear); |
|
Even CI is happy. Merging. |
Pull request overview
As described in #7173, the leap year flag in the weather file was overriding inputs in certain circumstances. The culprit was a bad bit of logic in WeatherManager introduced in the year awareness work.
Work Checklist
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Review Checklist
This will not be exhaustively relevant to every PR.