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

Revert "Adopt the Extended Heat Index calculation in zone resilience" #10732

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

Myoldmopar
Copy link
Member

The develop branch hit a pretty severe runtime slow down, and it appears it was because of this. I'm opening this PR to confirm and then we'll decide what to do. FYI @yujiex

@Myoldmopar Myoldmopar added the DoNotPublish Includes changes that shouldn't be reported in the changelog label Sep 12, 2024
@Myoldmopar Myoldmopar self-assigned this Sep 12, 2024
@Myoldmopar
Copy link
Member Author

Yep, check here: https://myoldmopar.github.io/EnergyPlusBuildResults/EnergyPlus-a0f92d6fdf8a9666076dee4c916113d42225b6d0-x86_64-Linux-Ubuntu-22.04-gcc-11.4.html#performance

This is indeed causing the big slow down. @yujiex if you want to come up with a very quick way to fix the HI code so that it doesn't cause any slowdown, you can, but I'm likely to merge this to get a clean baseline again.

Copy link

⚠️ Regressions detected on macos-14 for commit bf35cb8

Regression Summary
  • ESO Big Diffs: 4
  • Table Big Diffs: 3

@Myoldmopar
Copy link
Member Author

OK, I'm just going to merge this in. @yujiex we can chat about strategies to get the code back in place to go in after release. Again, if you want to find a way to get it in very quickly (today?) in a way that doesn't slow down our tests, I'm open to it. But I want to get develop's performance cleaned back up.

@Myoldmopar Myoldmopar merged commit 39ffd44 into develop Sep 12, 2024
11 checks passed
@Myoldmopar Myoldmopar deleted the revert-10548-extendedHI branch September 12, 2024 15:47
@yujiex
Copy link
Collaborator

yujiex commented Sep 12, 2024

I will attempt to fix it today @Myoldmopar

@yujiex
Copy link
Collaborator

yujiex commented Sep 12, 2024

@Myoldmopar. I've tried changing the root finder to the following. The unit tests still passes. So it seems this doesn't affect the result much but limit the number of iterations to 5 (or according to user input for the HVACSystemRootFindingAlgorithm. Do you feel this could resolve the problem?

state.dataRootFinder->HVACSystemRootFinding.HVACSystemRootSolver = HVACSystemRootSolverAlgorithm::BisectionThenRegulaFalsi;

@Myoldmopar
Copy link
Member Author

You are welcome to push it as a new branch with those changes made and see how CI responds. If it's happy there, I'll do a quick additional test locally and if it's all clean and fast enough, it can surely drop right in.

@yujiex
Copy link
Collaborator

yujiex commented Sep 12, 2024

Sure. Let me push to a new branch

@rraustad
Copy link
Contributor

rraustad commented Sep 12, 2024

I think you want to use RegulaFalsiThenBisection. Bisection already proved to give poor performance. You could always try both and choose the fastest option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DoNotPublish Includes changes that shouldn't be reported in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants