-
Notifications
You must be signed in to change notification settings - Fork 461
Correct Time Stamp Reported in EDD #11139
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
Changes from all commits
516ae1c
707227d
814281f
a495bd9
6f13dcb
31cfc2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -697,4 +697,59 @@ TEST_F(EnergyPlusFixture, General_findReportPeriodIdx) | |
| EXPECT_FALSE(reportPeriodFlags(1)); | ||
| EXPECT_FALSE(reportPeriodFlags(2)); | ||
| } | ||
|
|
||
| TEST_F(EnergyPlusFixture, General_CreateSysTimeIntervalString_Test) | ||
| { | ||
| // Unit test created as part of fix for Issue #10801 | ||
| auto &dHVACG = state->dataHVACGlobal; | ||
| auto &dGlo = state->dataGlobal; | ||
| std::string resultingString; | ||
| std::string expectedString; | ||
|
|
||
| // NOTE: SysTimeElapsed is updated at the END of the HVAC time step (loop), so it's current value is | ||
| // the end of the last HVAC time step not the end of the current HVAC time step. The other | ||
| // conditions below are for when we are in the zone heat balance (SysTimeElapsed = 0) or after | ||
| // we have finished the last HVAC time step. | ||
|
|
||
| // Test 1: Middle of the day/middle of zone timestep (system time step less than zone timestep) | ||
| dHVACG->SysTimeElapsed = 0.10; | ||
| dHVACG->TimeStepSys = 0.05; | ||
| dGlo->CurrentTime = 10.6; | ||
| dGlo->TimeStepZone = 0.2; | ||
| expectedString = "10:30 - 10:33"; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CurrentTime = 10.6 or 10:36 and TimeStepZone = 0.2 or 12 minutes, so at zone level this is time 10:24 - 10:36. This relates to the else. It may be that ActualTimeS is really ActualTimeE and then ActualTimeS = ActualTimeE - TimeStepSys; I would think this would show up the the csv and edd so you could quickly find out which is correct.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'm not sure why the comments I just made at the line are not showing up here...or why only half of your comment here shows up at the comment at Line 714. I'll paste in here and then respond to the second half of your comment about the else statement. SysTimeElapsed gets updated at the end of the system time step. So, during a particular HVAC time step, while it is still simulating something, SysTimeElapsed is the time elapsed to the end of the last time step. So, in this case, because SysTimeElapsed is set to 0.1 and is not equal to the TimeStepZone, we are still simulating HVAC time steps, we have finished the time step that ends at SysTimeElapsed, and we are now in the NEXT HVAC time step. So, we have already simulated through 10:30 and are now in the 10:30-10:33 time frame. Does that make sense? Now, what I'm not sure about is why when unix runs this that it thinks the answer should be 10:33-10:36. I think maybe because I used abs rather than std::abs, but that seems like a stretch. In any case, I did correct that and make another commit. Guess we'll see what comes back. So far, in the last hour+, I'm not getting any reports that the unit test is failing so I'm slightly hopeful. Whew...unit test on unix is now coming back all green. Good thing because I had absolutely no other explanation as to why it would come back different on different operating systems. 😳😂 Ok, now about your else comment in the second half. The else condition is identical to what is being used in develop and I believe that is what we want. There weren't problems in develop with error messages so it seemed right to leave that as-is (this was a backtrack from the first version I committed after thinking this through again at your suggestion). And I believe that it is actually correct because SysTimeElapsed is not the end of the current HVAC time step but rather the previous one because it gets updated at the end of the HVAC time step. This hopefully gets rid of the .err file errors that showed up after the first commit where error messages that use this time stamp routine were shifted. That's not what we wanted, I think. |
||
|
|
||
| resultingString = CreateSysTimeIntervalString(*state); | ||
| EXPECT_EQ(resultingString, expectedString); | ||
|
|
||
| // Test 2: Beginning of the day/hour/timestep | ||
| dHVACG->SysTimeElapsed = 0.10; | ||
| dHVACG->TimeStepSys = 0.10; | ||
| dGlo->CurrentTime = 0.10; | ||
| dGlo->TimeStepZone = 0.10; | ||
| expectedString = "00:00 - 00:06"; | ||
|
|
||
| resultingString = CreateSysTimeIntervalString(*state); | ||
| EXPECT_EQ(resultingString, expectedString); | ||
|
|
||
| // Test 3: End of the day/hour/timestep | ||
| dHVACG->SysTimeElapsed = 0.2; | ||
| dHVACG->TimeStepSys = 0.2; | ||
| dGlo->CurrentTime = 24.0; | ||
| dGlo->TimeStepZone = 0.2; | ||
| expectedString = "23:48 - 24:00"; | ||
|
|
||
| resultingString = CreateSysTimeIntervalString(*state); | ||
| EXPECT_EQ(resultingString, expectedString); | ||
|
|
||
| // Test 4: SysTimeElapsed zero -- before HVAC simulation sets it | ||
| dHVACG->SysTimeElapsed = 0.0; | ||
| dHVACG->TimeStepSys = 0.1; | ||
| dGlo->CurrentTime = 10.9; | ||
| dGlo->TimeStepZone = 0.1; | ||
| expectedString = "10:48 - 10:54"; | ||
|
|
||
| resultingString = CreateSysTimeIntervalString(*state); | ||
| EXPECT_EQ(resultingString, expectedString); | ||
| } | ||
|
|
||
| } // namespace EnergyPlus | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RKStrand what an exciting PR! I appreciate your efforts and @rraustad scrutinizing this. I'm glad CI is coming out so clean with only a few unit test fixups.