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.

Sign up for GitHub

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

Jump to bottom

Fix #10399 - Long elapsed times are not recorded correctly #10529

Merged
merged 5 commits into from
Jun 3, 2024
Merged

Fix #10399 - Long elapsed times are not recorded correctly #10529

merged 5 commits into from
Jun 3, 2024

Conversation

jmarrec
Copy link
Contributor

@jmarrec jmarrec commented May 28, 2024

Pull request overview

I implemented a Timer class, that I made re-startable (even though not needed right now) like a stopwatch, in case we do want to use that for timings specific bits of the code.

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

@jmarrec jmarrec added the Defect Includes code to repair a defect in EnergyPlus label May 28, 2024
@jmarrec jmarrec requested a review from Myoldmopar May 28, 2024 08:54
@jmarrec jmarrec self-assigned this May 28, 2024
Myoldmopar
Myoldmopar approved these changes May 28, 2024
View reviewed changes
Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

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

Yeah this is nice. I'll do a quick build and check CI, but it's good here.

src/EnergyPlus/CMakeLists.txt
@@ -602,6 +602,8 @@ set(SRC
TARCOGParams.hh
TarcogShading.cc
TarcogShading.hh
Timer.cc
Timer.hh
Copy link
Member

Choose a reason for hiding this comment

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

Hey I've seen that before!

https://github.com/NREL/EnergyPlusArchive/blob/master/SourceCode/Timer.h

tst/EnergyPlus/unit/Timer.unit.cc
t.m_start =
Timer::ClockType::now() - (std::chrono::hours(2) + std::chrono::minutes(25) + std::chrono::seconds(51) + std::chrono::milliseconds(341));
t.tock();
EXPECT_EQ("02hr 25min 51.34sec", t.formatAsHourMinSecs());
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm... This feels... Probably fine...

@Myoldmopar
Copy link
Member

Myoldmopar commented May 28, 2024 •
edited
Loading

Confirmed everything is 💯 here, merging. Thanks @jmarrec

jmarrec
jmarrec commented May 28, 2024
View reviewed changes
src/EnergyPlus/Timer.cc
std::string Timer::formatAsHourMinSecs() const
{
// We're working with ms as base.
// TODO: do we want to report elapsed time of 25 hours as 1 day 1hr 0min 0sec or 25hr 0min 0sec?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Myoldmopar forgot to flag this
Do we want 1 days 1 hr xxx or 2hr

Copy link
Member

Choose a reason for hiding this comment

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

I do not care... Possibly 1 day 1 hr ?? I think either is fine though. This would only matter in a corner case where (runtime is **enormously** long) .and. (the user cant handle a 25 hour time stamp) which should round down to zero percent of the EnergyPlus use cases. Anyway, whatever is happening right now is fine.

jmarrec
jmarrec commented May 29, 2024
View reviewed changes
src/EnergyPlus/Timer.cc
Comment on lines +86 to +93
// We're working with ms as base.
// TODO: do we want to report elapsed time of 25 hours as 1 day 1hr 0min 0sec or 25hr 0min 0sec?
// constexpr auto num_ms_in_day = 1000 * 3600 * 24;
// if (duration().count() > num_ms_in_day) {
// return fmt::format("{:.2%j days %Hhr %Mmin %Ssec}\n", duration());
// } else {
// return fmt::format("{:%Hhr %Mmin %Ssec}\n", duration());
// }
Copy link
Contributor Author

@jmarrec jmarrec May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@Myoldmopar Assuming we want to keep 25 hr as an output format, just apply this:

Suggested change
// We're working with ms as base.
// TODO: do we want to report elapsed time of 25 hours as 1 day 1hr 0min 0sec or 25hr 0min 0sec?
// constexpr auto num_ms_in_day = 1000 * 3600 * 24;
// if (duration().count() > num_ms_in_day) {
// return fmt::format("{:.2%j days %Hhr %Mmin %Ssec}\n", duration());
// } else {
// return fmt::format("{:%Hhr %Mmin %Ssec}\n", duration());
// }
// We're working with ms as base.

jmarrec
jmarrec commented May 30, 2024
View reviewed changes
tst/EnergyPlus/unit/Timer.unit.cc Outdated
Comment on lines 77 to 78
EXPECT_EQ(124, t.duration().count());
EXPECT_EQ(0.124, t.elapsedSeconds());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just realized that Marvin actually reported 128. I'll adjust the test, we don't want it to fail when the system is busy.

It's really hidden to see the green check mark and just get an email. @Myoldmopar

  • Do we have a test failure threshold or something? Is that warranted?
  • We really need to bring back PR annotations

image

https://myoldmopar.github.io/EnergyPlusBuildResults/EnergyPlus-61cf2187918f76746b9eaa263db904f4aeffed56-x86_64-MacOS-10.18-clang-15.0.0.html

Copy link
Member

Choose a reason for hiding this comment

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

For test failure threshold, we should have it set to fail if any unit test fails. If regressions fail, that will be fine, but we should still see the warnings. I'm still upset at GH for just disabling commit comments on the PR page without an easy way to bring them back.

IIRC, in my development branch of Decent CI, I operate on PRs directly where possible. So it's easy to move the comments to directly on the PRs. I'm open to pushing on that again, it's just a big distraction.

jmarrec added a commit that referenced this pull request May 30, 2024
@jmarrec
Relax tolerance for timer test
8d6d7a2 
cf #10529 (comment)
@jmarrec jmarrec force-pushed the 10399_LongElapsedTimes branch from 61cf218 to 8d6d7a2 Compare May 30, 2024 11:15
@jmarrec jmarrec force-pushed the 10399_LongElapsedTimes branch from 8d6d7a2 to 8776c96 Compare May 30, 2024 15:15
@jmarrec
Relax more
b2ecb40 
Obviously if I were to sleep for like 10 seconds, the % difference would be much smaller. But this is a dumb test, and it's the fourth time I push (I squashed earlier)
@Myoldmopar
Copy link
Member

This is all happy, let's get it in to get today's merge queue moving. Thanks for adding the robustness to the time test, @jmarrec

@Myoldmopar Myoldmopar merged commit 9bf79d8 into develop Jun 3, 2024
15 checks passed
@Myoldmopar Myoldmopar deleted the 10399_LongElapsedTimes branch June 3, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Myoldmopar Myoldmopar Myoldmopar approved these changes

Assignees

@jmarrec jmarrec

Labels
Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Long elapsed times are not recorded correctly
7 participants
@jmarrec @Myoldmopar @nrel-bot @nrel-bot-2 @nrel-bot-3 @nrel-bot-2b @nrel-bot-2c