Skip to content
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

Fix Rainfall is handled differently between weather file values and Site:Precipitation water manager values #4153 #9290

Merged
merged 25 commits into from
Mar 22, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
62d6c9b
Initial commit
yujiex Feb 22, 2022
663087a
Revert idd field renaming
yujiex Feb 22, 2022
272ecb7
Move monthly rain accumulator into UpdatePrecipitation
yujiex Feb 22, 2022
2ed751a
Merge remote-tracking branch 'origin/develop' into fixRainFlag
yujiex Feb 22, 2022
776ca6e
fix type
yujiex Feb 22, 2022
2f8f8ad
Unit test of rain flag set by site:precipitation
yujiex Feb 23, 2022
5e143d8
clang-format
yujiex Feb 23, 2022
9441a0e
Change lowerbound of isRain to match TommorrowIsRain
yujiex Feb 23, 2022
f0e0f03
Merge remote-tracking branch 'origin/develop' into fixRainFlag
yujiex Mar 3, 2022
b042c39
Merge remote-tracking branch 'origin/develop' into fixRainFlag
yujiex Mar 14, 2022
63a2f62
Rain flag follow dev branch with >=
yujiex Mar 14, 2022
77df693
Lower bound following dev differ by timestep=1
yujiex Mar 16, 2022
5770807
Move monthly prec table col 1 calc back to weather manager
yujiex Mar 16, 2022
be22899
Merge remote-tracking branch 'origin/develop' into fixRainFlag
yujiex Mar 16, 2022
9b30cb7
Add more conditions in isRain calculation
yujiex Mar 16, 2022
8344d8d
Update unit test
yujiex Mar 16, 2022
b4b8a92
Merge remote-tracking branch 'origin/develop' into fixRainFlag
yujiex Mar 16, 2022
5e43004
Remove redundant thresholding
yujiex Mar 17, 2022
59712dd
Update doc
yujiex Mar 17, 2022
724d4c2
Merge remote-tracking branch 'origin/develop' into fixRainFlag
yujiex Mar 17, 2022
a41d31a
Comments 03-21
yujiex Mar 21, 2022
45e8e83
Merge remote-tracking branch 'origin/develop' into fixRainFlag
yujiex Mar 21, 2022
100e3c5
More doc fixes about rain indicator overwritten
yujiex Mar 21, 2022
64c2e23
Update unit test, change rain flag overwritten from ifelse to if
yujiex Mar 21, 2022
9004c94
Add back the else to explicitly set IsRain to false
yujiex Mar 21, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion idd/Energy+.idd.in
Original file line number Diff line number Diff line change
Expand Up @@ -1442,7 +1442,7 @@ RunPeriod,
\key Yes
\key No
\default No
A6, \field Use Weather File Rain Indicators
A6, \field Use Rain Indicators
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that I/O Freeze is almost frozen, the IDD change will likely have to wait until after v22.1 release. And it will need doc updates and a mention in the input rule file (because field name changes must be documented for epJSON).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove the I/O change for now.

\type choice
\key Yes
\key No
Expand Down
26 changes: 14 additions & 12 deletions src/EnergyPlus/WeatherManager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2309,33 +2309,35 @@ namespace WeatherManager {
if (state.dataEnvrn->EMSBeamSolarRadOverrideOn) state.dataEnvrn->BeamSolarRad = state.dataEnvrn->EMSBeamSolarRadOverrideValue;
state.dataEnvrn->LiquidPrecipitation =
state.dataWeatherManager->TodayLiquidPrecip(state.dataGlobal->TimeStep, state.dataGlobal->HourOfDay) / 1000.0; // convert from mm to m
if ((state.dataEnvrn->RunPeriodEnvironment) && (!state.dataGlobal->WarmupFlag)) {
int month = state.dataEnvrn->Month;
state.dataWaterData->RainFall.MonthlyTotalPrecInWeather.at(month - 1) += state.dataEnvrn->LiquidPrecipitation * 1000.0;
if ((state.dataEnvrn->LiquidPrecipitation > 0) && (state.dataGlobal->TimeStep == 1)) {
state.dataWaterData->RainFall.numRainyHoursInWeather.at(month - 1) += 1;
}
}

// if using epw precipitation, throw warning
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this now, it would be clearer (and save a couple of if blocks) to move this (lines 2313 thru 2332) into `WaterManager::UpdatePrecipitation. Review that function for unnecessary if blocks as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I combined this with the if statement in UpdatePrecipitation

if (state.dataWaterData->RainFall.ModeID == DataWater::RainfallMode::EPWPrecipitation) {
if ((state.dataEnvrn->LiquidPrecipitation == 0) && (state.dataEnvrn->IsRain)) {
state.dataEnvrn->LiquidPrecipitation = 1.5 / 1000.0;
if ((state.dataEnvrn->LiquidPrecipitation == 0) &&
// this is the Present Weather Codes field in epw. See column 1-3 in Table 2.16 Weather Codes Field Interpretation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's confusing to have a comment in the middle of the if statement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved the comments above the if statement.

(state.dataWeatherManager->TodayIsRain(state.dataGlobal->TimeStep, state.dataGlobal->HourOfDay)) &&
(state.dataWeatherManager->UseRainValues)) {
state.dataEnvrn->LiquidPrecipitation = 1.5 / 1000.0 / state.dataGlobal->TimeStepZone;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NumOfTimeStepInHour instead of TimeStepZone here. Also, the literal 1.5/1000 should be declared here as a constexpr variable, named something like defaultLiquidPrecip.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I should have it divided by NumOfTimeStepInHours. I just added the defaultLiquidPrecip to hold the constant.

ShowRecurringWarningErrorAtEnd(state,
"Rain flag is on but precipitation depth in the weather file is missing or zero. Setting "
"precipitation depth to 1.5 mm for this hour.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this can be deleted, because the earlier weather processing has already set this to 2mm/hr here and then adjusted for number of timesteps here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this will not be needed. I deleted it.

state.dataWaterData->PrecipOverwrittenByRainFlag);
}
}
if ((state.dataEnvrn->RunPeriodEnvironment) && (!state.dataGlobal->WarmupFlag)) {
int month = state.dataEnvrn->Month;
state.dataWaterData->RainFall.MonthlyTotalPrecInWeather.at(month - 1) += state.dataEnvrn->LiquidPrecipitation * 1000.0;
if ((state.dataEnvrn->LiquidPrecipitation > 0) && (state.dataGlobal->TimeStep == 1)) {
state.dataWaterData->RainFall.numRainyHoursInWeather.at(month - 1) += 1;
}
}

WaterManager::UpdatePrecipitation(state);

state.dataEnvrn->TotalCloudCover = state.dataWeatherManager->TodayTotalSkyCover(state.dataGlobal->TimeStep, state.dataGlobal->HourOfDay);
state.dataEnvrn->OpaqueCloudCover = state.dataWeatherManager->TodayOpaqueSkyCover(state.dataGlobal->TimeStep, state.dataGlobal->HourOfDay);

if (state.dataWeatherManager->UseRainValues) {
state.dataEnvrn->IsRain = state.dataWeatherManager->TodayIsRain(
state.dataGlobal->TimeStep, state.dataGlobal->HourOfDay); //.or. LiquidPrecipitation >= .8d0) ! > .8 mm
Comment on lines -2337 to -2338
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep this code but add if (state.dataWaterData->RainFall.ModeID == DataWater::RainfallMode::EPWPrecipitation) and add a comment pointing to the code that sets TomorrowIsRain based on the 0.8mm/hr threshold.

state.dataEnvrn->IsRain = state.dataWaterData->RainFall.CurrentAmount > 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ever possible for the weather file data to have a non-zero (small) value for LiquidPrecipitation but TodayIsRain is false? If so, this will change old behavior and flip the rain flag to true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll double-check whether this could happen. The other behavior change would be if weather data has TodayIsRain = false but user-input site:precipitation defined non-zero rainfall value.

Copy link
Contributor

@mjwitte mjwitte Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other behavior change would be if weather data has TodayIsRain = false but user-input site:precipitation defined non-zero rainfall value.

True, but that's the whole point of this PR in the first place. So that's a good change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked the TMY3 data in the EnergyPlus/weather folder. All of them have PresWeathCodes = 999999999. As a result, none of them can set TodayIsRain to true. I'll download some more weather data and see.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjwitte I also downloaded 36 more TMY3 whose states are not covered by the weather files in EnergyPlus folder. All of them also have PresWeathCodes = 999999999.

} else {
state.dataEnvrn->IsRain = false;
}
Expand Down Expand Up @@ -5646,7 +5648,7 @@ namespace WeatherManager {
ErrorsFound = true;
}

// A6, \field Use Weather File Rain Indicators
// A6, \field Use Rain Indicators
if (state.dataIPShortCut->lAlphaFieldBlanks(6) || UtilityRoutines::SameString(state.dataIPShortCut->cAlphaArgs(6), "YES")) {
state.dataWeatherManager->RunPeriodInput(i).useRain = true;
} else if (UtilityRoutines::SameString(state.dataIPShortCut->cAlphaArgs(6), "NO")) {
Expand Down
8 changes: 4 additions & 4 deletions tst/EnergyPlus/unit/InputProcessor.unit.cc
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,7 @@ TEST_F(InputProcessorFixture, parse_two_RunPeriod)
" Yes, !- Use Weather File Holidays and Special Days",
" Yes, !- Use Weather File Daylight Saving Period",
" No, !- Apply Weekend Holiday Rule",
" Yes, !- Use Weather File Rain Indicators",
" Yes, !- Use Rain Indicators",
" Yes; !- Use Weather File Snow Indicators",
"",
" RunPeriod,",
Expand All @@ -713,7 +713,7 @@ TEST_F(InputProcessorFixture, parse_two_RunPeriod)
" Yes, !- Use Weather File Holidays and Special Days",
" Yes, !- Use Weather File Daylight Saving Period",
" No, !- Apply Weekend Holiday Rule",
" Yes, !- Use Weather File Rain Indicators",
" Yes, !- Use Rain Indicators",
" Yes; !- Use Weather File Snow Indicators",
}));

Expand All @@ -727,7 +727,7 @@ TEST_F(InputProcessorFixture, parse_two_RunPeriod)
{"end_month", 1},
{"use_weather_file_daylight_saving_period", "Yes"},
{"use_weather_file_holidays_and_special_days", "Yes"},
{"use_weather_file_rain_indicators", "Yes"},
{"use_rain_indicators", "Yes"},
{"use_weather_file_snow_indicators", "Yes"}}},
{"SummerDay",
{{"apply_weekend_holiday_rule", "No"},
Expand All @@ -738,7 +738,7 @@ TEST_F(InputProcessorFixture, parse_two_RunPeriod)
{"end_month", 7},
{"use_weather_file_daylight_saving_period", "Yes"},
{"use_weather_file_holidays_and_special_days", "Yes"},
{"use_weather_file_rain_indicators", "Yes"},
{"use_rain_indicators", "Yes"},
{"use_weather_file_snow_indicators", "Yes"}}}}}};

ASSERT_TRUE(process_idf(idf));
Expand Down