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

Enhancement of Handling of Rainfall #9177

Merged
merged 98 commits into from
Feb 19, 2022
Merged

Enhancement of Handling of Rainfall #9177

merged 98 commits into from
Feb 19, 2022

Conversation

yujiex
Copy link
Collaborator

@yujiex yujiex commented Nov 8, 2021

Pull request overview

The Site:Precipitation values are used in two places in EnergyPlus, namely the rain collector and the roof irrigation. The rain collector, modeled by WaterUse:RainCollector object, is used for harvesting rainwater falling on building surfaces. The rainwater is then sent to a WaterUse:Storage object. Currently, the use of the rain collector object requires the inclusion of a Site:Precipitation object to describe the rates of rainfall. In the green roof module, the soil energy heat released or gained is calculated based on the phase changes of soil water, precipitation heat flux, and heat flux due to vertical transport of water in the soil are ignored. The precipitation value is added to the surface soil moisture variable if a Site:Precipitation schedule exists. In particular, the green roof run-off rate is calculated based on the irrigation rate on top of the precipitation rate.

As local weather stations are providing hourly precipitation for the past decades and the values are being updated in TMY3 data, we propose to modify the logic of using rainfall/liquid precipitation to use the EPW file hourly precipitation data for rain harvesting and green roof when no Site:Precipitation schedule exists. In EPW files, the field of “Liquid Precipitation Depth” holds the amount of liquid precipitation (mm) observed at the indicated time for the period indicated in the liquid precipitation quantity field. Used in EnergyPlus simulation, if this value is not missing, it is used and overrides the “precipitation” flag as rainfall. Conversely, if the precipitation flag shows rain and this field is missing or zero, it is set to 1.5 (mm).

The memo of the WaterUse:RainCollector will be modified to reflect changes in the requirement of a Site:Precipitation schedule.

This feature also fixed a lagging issue of the surface soil moisture calculation, where precipitation value is updated after it is used in moisture calculation. As a result, the moisture calculation uses leftover precipitation from the previous timestep. This feature fixed this issue by moving the precipitation update to WeatherManager, which is executed before the surface soil moisture adds in the precipitation term.

Addresses #4714

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

@yujiex yujiex added NewFeature Includes code to add a new feature to EnergyPlus IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) labels Nov 8, 2021
@yujiex
Copy link
Collaborator Author

yujiex commented Nov 8, 2021

Hello @JasonGlazer @Myoldmopar @mjwitte, would you be available to review this rainfall handling PR and provide some feedback? Thanks :)

## Approach ##

When a *WaterUse:RainCollector* or a RoofIrrigation object is defined without a *Site:Precipitation* object presented, we will use the hourly precipitation value defined in the EPW file instead. When every entry of the liquid precipitation depth is missing in the 8760 hourly period, we will throw a warning and use the precipitation flag to estimate the precipitation quantity (as 1.5mm).

Copy link
Contributor

@JasonGlazer JasonGlazer Nov 10, 2021

Choose a reason for hiding this comment

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

I believe many EPW files are from sources where the rain amount is poorly estimated or unknown so I think there should be some protection for users in this case. I would suggest when no Site:precipitation object is present, but WaterUse:RainCollector or RoofIrrigation or ecoroof objects are present, a warning message should be added. The warning message should provide the annual total precipitation that is shown in the EPW file. It would be even better if some guidance was provided in the warning message if this value in the EPW file is reasonable or not. Perhaps normal ranges based on the climate zone or climate type based on the stat file could provide guidance (although this is also often missing) or some other factor from the EPW file could indicate how wet that climate typically is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. A warning message could be added to indicate the fact that the weather file will be used in RainCollector or RoofIrrigation calculation instead of Site:precipitation. Other warnings could also be included to flag out-of-normal-range rainfall data in the weather file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @JasonGlazer, I added some descriptions in the approach section about the warning messages when EPW is used in RainCollection or Roof Irrigation calculation and when EPW precipitation data is out of range. The "out of range" is defined as the EPW hourly precipitation value being larger than the state recorded highest 24-hour precipitation, according to this table. I haven't found the hourly extreme yet, so I was considering using this 24-hour one as an initial upper bound. I'll see if I can find the hourly extreme record per state or climate zone and replace this upper bound later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @JasonGlazer, just to confirm. Is it better to throw a warning when using epw in RainCollection or RoofIrrigation calculation, as well as provide a link of this table in the documentation so that users can check the epw precipitation range before feeding it into the simulation? Or should I also include this table in the code and check the rainfall data range according to this table in EnergyPlus? I'm leaning towards the former as there might need some continuous effort in the future to maintain this table up-to-date. Any thoughts?

@mjwitte
Copy link
Contributor

mjwitte commented Nov 18, 2021

I would suggest that annual precipitation be reported in the "Climatic Data Summary" table output. The values could come from the weather.stat file, but it may be better to accumulate a total based on the actual weather data used (including any missing values that are assumed to be 1.5mm). I suppose you could go one step further and report hours of rainfall or make a monthly table, and if a Site:Precipitation object is present, the same statistics could be reported for that instead of from the weather file? Just brainstorming here, but basically suggesting some reporting of precipitation amount would be very useful.

@yujiex
Copy link
Collaborator Author

yujiex commented Dec 6, 2021

I would suggest that annual precipitation be reported in the "Climatic Data Summary" table output. The values could come from the weather.stat file, but it may be better to accumulate a total based on the actual weather data used (including any missing values that are assumed to be 1.5mm). I suppose you could go one step further and report hours of rainfall or make a monthly table, and if a Site:Precipitation object is present, the same statistics could be reported for that instead of from the weather file? Just brainstorming here, but basically suggesting some reporting of precipitation amount would be very useful.

Thanks for the suggestion. Sorry for the late response. Indeed it will be nice to have some rainfall output so that we can see more clearly the rainfall used in the calculation. I'll add rainfall summary outputs to the "Climatic Data Summary" table.

@yujiex
Copy link
Collaborator Author

yujiex commented Dec 8, 2021

Note: the ERR diffs are intentional. They are the added warning messages when the .epw precipitation data is used in the rain collector or roof irrigation calculation.

@mjwitte
Copy link
Contributor

mjwitte commented Feb 17, 2022

For the second one, the rain flag is updated every timestep, maybe the warning should also be at the timestep level?

Sure, or maybe just make is a simple one-time warning and add "Further instances of this condition will not be reported."? We've done that with some others. If you like that approch, add a ShowContinueErrorTimeStamp to indicate the time of the warning.

@yujiex
Copy link
Collaborator Author

yujiex commented Feb 17, 2022

For the second one, the rain flag is updated every timestep, maybe the warning should also be at the timestep level?

Sure, or maybe just make is a simple one-time warning and add "Further instances of this condition will not be reported."? We've done that with some others. If you like that approch, add a ShowContinueErrorTimeStamp to indicate the time of the warning.

Thanks for the pointer. I'll check that out.

@yujiex
Copy link
Collaborator Author

yujiex commented Feb 17, 2022

For the second one, the rain flag is updated every timestep, maybe the warning should also be at the timestep level?

Sure, or maybe just make is a simple one-time warning and add "Further instances of this condition will not be reported."? We've done that with some others. If you like that approch, add a ShowContinueErrorTimeStamp to indicate the time of the warning.

Thanks for the pointer. I'll check that out.

On second thought, would users want to know how many times this type of overwritten happened? If there are a lot, maybe they want to look into the data?

@@ -317,6 +329,9 @@ struct DataWaterData : BaseGlobalStruct
bool AnyWaterSystemsInModel = false; // control flag set true if any water systems
bool WaterSystemGetInputCalled = false; // set true once input data gotten.
bool AnyIrrigationInModel = false; // control flag set true if irrigation input for ecoroof DJS PSU Dec 2006
bool UsePrecipitation = false; // whether to include the rain fall amount in the actual irrigation amount
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not used now.

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 forgot to modify this header file

Comment on lines 930 to 935
} else { // no schedule
if (state.dataWaterData->RainFall.ModeID == DataWater::RainfallMode::EPWPrecipitation) {
// irrigation is just the rain amount
state.dataEcoRoofMgr->CurrentIrrigation = state.dataEcoRoofMgr->CurrentPrecipitation; // units of m
state.dataWaterData->Irrigation.ActualAmount = state.dataEcoRoofMgr->CurrentIrrigation;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be here. Precipitation is added above.

Copy link
Contributor

Choose a reason for hiding this comment

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

The irrigation never operates in EcoroofOrlando (Environment:Green Roof Cumulative Irrigation Depth m is always zero, in develop and in this branch). Let's not change that here, but we should do a followup PR to make this a better example file (with the same changes in EcoroofOrlando_NoSitePrec).

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 got confused earlier, CurrentIrrigation should not add precipitation.

@@ -713,6 +724,14 @@ namespace WaterManager {

} // NumIrrigation ==1

if (state.dataWaterData->RainFall.ModeID == DataWater::RainfallMode::EPWPrecipitation) {
ShowRecurringWarningErrorAtEnd(
Copy link
Contributor

Choose a reason for hiding this comment

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

ShowWarningError (not at end)

state.dataWaterData->RainFall.CurrentRate,
OutputProcessor::SOVTimeStepType::System,
OutputProcessor::SOVStoreType::Average,
"Site:Precipitation");
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be "Environment" here and in the next one. Would you agree? Because these are now the actual environment values whether Site:Precipitation is used or not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True. I will make them "Environment".

Copy link
Contributor

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

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

Almost there. Thanks for all of the changes and reversals.

Looking at 5ZoneWaterSystems, something seems amiss with Site Precipitation Depth. For example, on 01/03 hour 22, the values with develop are 0.0001616 but with this branch, it's 0.0003232. The sum over the year goes from 0.749 to 1.282. Since this file has Site:Precipitation, I would expect these to be unchanged.

@@ -1831,6 +1847,13 @@ \subsubsection{Inputs}\label{inputs-23-002}

Name of a schedule defined elsewhere that describes the rate of irrigation. The values in this schedule are the average rate of irrigation in meters per hour.

\paragraph{Field: Use Precipitation}\label{field-use-precipitation}
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be deleted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for spotting this, I just deleted the "Use Precipitation" related documentation.

@yujiex
Copy link
Collaborator Author

yujiex commented Feb 18, 2022

I'll look

I add the following output variable in idf and compared the eplusout.csv

Output:Variable,*,Site Precipitation Depth,hourly;
The following is what I see (the second column is the develop branch, the third column is this feature branch). Strangely develop branch shows zero for 01/03 all day. I'll try to do a fresh clone and redo the comparison.
image

From the idf, the site:precipitation schedule value should be like this, which is not the case for the feature branch. I'll debug this part as well.

    Through: 1/3,            !- Field 11
    For: AllDays,            !- Field 12
    Until: 3:00,0.000696148, !- Field 13
    Until: 11:00,0,          !- Field 15
    Until: 13:00,0.000696148,!- Field 17
    Until: 15:00,0,          !- Field 19
    Until: 16:00,0.000696148,!- Field 21
    Until: 21:00,0,          !- Field 23
    Until: 22:00,0.000696148,!- Field 25
    Until: 24:00,0,          !- Field 27

@@ -2292,6 +2309,27 @@ 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this line should include && !Warmup ?

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 add it in the && !Warmup. This part is used in reporting the precipitation-related quantities during the actual simulation period in the HTML tabular report. It should not include the warmup period.

Copy link
Contributor

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

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

Confirmed that Precipitation Depth values are as expected now in 5ZoneWaterSystems. Will wait for CI to report back in bright green, then merge.

@yujiex
Copy link
Collaborator Author

yujiex commented Feb 18, 2022

Confirmed that Precipitation Depth values are as expected now in 5ZoneWaterSystems. Will wait for CI to report back in bright green, then merge.

Fingers crossed

@yujiex
Copy link
Collaborator Author

yujiex commented Feb 19, 2022

The current diffs in 5ZoneWaterSystems.idf seems to be from the change in the idf file in this branch 5ZoneWaterSystems.idf.

@yujiex
Copy link
Collaborator Author

yujiex commented Feb 19, 2022

The math and table diffs in EcoroofOrlando is due to the lagging issue in the develop branch.
I tried adding the following to mimic the lagging behavior, then the diffs are gone.
image

@mjwitte
Copy link
Contributor

mjwitte commented Feb 19, 2022

Not sure why the CI comments for 1a3478f are not showing here, but they're all good. Merging.

@mjwitte mjwitte merged commit d48b30c into develop Feb 19, 2022
@mjwitte mjwitte deleted the enhanceRainfallHandling branch February 19, 2022 16:43
@yujiex
Copy link
Collaborator Author

yujiex commented Feb 21, 2022

Not sure why the CI comments for 1a3478f are not showing here, but they're all good. Merging.

Thanks, @mjwitte :) Glad it all run through well. Thanks for all the help and guidance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) NewFeature Includes code to add a new feature to EnergyPlus OutputChange Code changes output in such a way that it cannot be merged after IO freeze
Projects
None yet