Daylight savings time adding incorrect month calculation in time of use tariff

JasonGlazer commented 4 years ago

Issue overview

The UtilityCost:Tariff object with a time of use period schedule and a season schedule that both start the summer on June 1 are showing summer tariff calculations starting in May. This file has a RunPeriodControl:DaylightSavingTime which starts daylight savings saving in March. When the RunPeriodControl:DaylightSavingTime is removed, the summer calculations are started in June when it is supposed to be.

Details

Some additional details for this issue (if relevant):

EnergyPlus 9.2.0
https://unmethours.com/question/42857/utility-tariff-not-calculating-midpeak-the-first-month/

Checklist

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

[x] Defect file added (devsupport)
[ ] Ticket added to Pivotal for defect (development team task)
[ ] Pull request created (the pull request will have additional tasks related to reviewing changes that fix this defect)

jmarrec commented 4 years ago

After fighting with my debugger, I think I've pinpointed something: it seems that ScheduleManager::GetCurrentScheduleValue returns something different than ScheduleManager::LookUpScheduleValues

To better illustrate my point, I've taken this block:

https://github.com/NREL/EnergyPlus/blob/36db22b9af45b33371b30b85de916659c091c7c8/src/EnergyPlus/EconomicTariff.cc#L2775-L2777

And replaced it with

if (tariff(iTariff).seasonSchIndex != 0) {
    curSeason = GetCurrentScheduleValue(tariff(iTariff).seasonSchIndex);
    if (curSeason == 3) {
       Real64 valueViaLookup = ScheduleManager::LookUpScheduleValue(
               tariff(iTariff).seasonSchIndex, DataGlobals::HourOfDay , DataGlobals::TimeStep);
       if (int(valueViaLookup) != curSeason) {
           ShowFatalError("GatherForEconomics: Something went terribly wrong");
       }
    }
}

Running with the defect file (or the MCVE I created) that has DST, and a season schedule like this:

Schedule:Compact,
    Electricity Season Schedule,  !- Name
    Any Number,              !- Schedule Type Limits Name
    Through: 5/31,           !- Field 1
    For: AllDays,            !- Field 2
    Until: 24:00,            !- Field 3
    1,                       !- Field 4
    Through: 9/30,           !- Field 5
    For: AllDays,            !- Field 6
    Until: 24:00,            !- Field 7
    3,                       !- Field 8
    Through: 12/31,          !- Field 9
    For: AllDays,            !- Field 10
    Until: 24:00,            !- Field 11
    1;                       !- Field 12

The Fatal is issued. GetCurrentScheduleValue returns 3 (summer), while LookUpScheduleValue returns correctly 1 (winter).

rraustad commented 4 years ago

This does ring a bell. Just gotta remember where.

rraustad commented 4 years ago

I was thinking of #7462. Doesn't seem the same as your simple schedule.

mjwitte commented 4 years ago

@rraustad It looks like the changes applied in #7462 were only made in UpdateScheduleValues (which feeds into GetCurrentScheduleValue) but not in LookupScheduleValue?

@jmarrec It would be helpful to know the day/hour/timestep when the fatal test trips.

jmarrec commented 4 years ago

@rraustad @mjwitte I isolated it in a unit test, cf https://github.com/NREL/EnergyPlus/commit/347707d58df48f225cd78c8278bd60b54bbd52a1

mjwitte commented 4 years ago

So, the question is which one is correct? #7462 would say that GetCurrentScheduleValue is the right answer. Clearly, both functions should return the same schedule value, but perhaps the economics calcs need to careful about when to select a monthly value? @JasonGlazer

jmarrec commented 4 years ago

ScheduleManager.cc has three routines with similar blocks of code. These two are exactly the same (minus whitespace changes)

ScheduleManager::ReportScheduleValues (called from HeatBalanceManager::ReportHeatBalance)
ScheduleManager::UpdateScheduleValues (called from WeatherManager::SetCurrentWeather)

This one is much different: ScheduleManager::LookUpScheduleValues. It seems be doing stuff twice on some occasions (control flow can probably be improved), and appears to add the DSTIndicator twice. I'm having a hard time following it right now, but will hopefully get it tomorrow morning after some rest.

https://github.com/NREL/EnergyPlus/blob/36db22b9af45b33371b30b85de916659c091c7c8/src/EnergyPlus/ScheduleManager.cc#L2922

https://github.com/NREL/EnergyPlus/blob/36db22b9af45b33371b30b85de916659c091c7c8/src/EnergyPlus/ScheduleManager.cc#L2954

jmarrec commented 4 years ago

This block appears to produce no meaningful actions as it's all overwritten later no matter the branches taken.

https://github.com/NREL/EnergyPlus/blob/36db22b9af45b33371b30b85de916659c091c7c8/src/EnergyPlus/ScheduleManager.cc#L2910-L2927

mjwitte commented 4 years ago

@jmarrec FYI, I wouldn't get too carried away with any refactoring here beyond fixing the issue. There's a major schedule manager refactor in progress #7352. Wonder how that branch handles this case?

JasonGlazer commented 4 years ago

@mjwitte the tariff stuff ought to add up the values for a time period with daylight savings time account for. Nothing special about that.

@jmarrec there does seem to be some redundancy in the code but be careful, there are a lot of edge cases.

mjwitte commented 4 years ago

@JasonGlazer What isn't clear here, is when in May the summer calcs are starting - is it just for that last hour on May 31?

JasonGlazer commented 4 years ago

I don't remember for sure but I assume it was just an hour off in May.

mjwitte commented 4 years ago

Then perhaps it's correct as-is. If the utility is collecting meter data hourly (or more often), then the last day of standard time will only have 23 hours of data, and the last day of DST would have 25. If I'm following this correctly, that seems to be what's happening here. There's still the issue of the two different functions returning different results, but that seems to be unrelated to tariff calcs.

jmarrec commented 4 years ago

The entire month of may ends up billed at the summer rate, so the error isn't just one hour off.

IIUC Tariff(n).seasonForMonth(CurMonth) is constantly overriden and only the last one to get in is used to compute the cost.

I'll try the ScheduleManager refactor branch tomorrow.

I do think GetScheduleValue and LookUpScheduleValue should return the same though. The different is the determination of the week/day schedule pointers, in one case(LookUp) it's done with HourOfDay then after determination is done DSTIndicator is added, for the other it's directly done with HourOfDay + DSTIndicator

NREL / EnergyPlus