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

Extended HI followup: fix speed issue #10733

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open

Extended HI followup: fix speed issue #10733

wants to merge 20 commits into from

Conversation

yujiex
Copy link
Collaborator

@yujiex yujiex commented Sep 12, 2024 •
edited
Loading

Pull request overview

  • This PR fixes the speed problem of root solver in extended HI calculation

A couple of different approaches were tried out to improve the performance

  • changing the root solver type from Bisection to BisectionThenRegulaFalsi: this had the biggest impact, bringing the damage from 70% to about 12%
  • result caching: saving Zs(Rs) result to a variable before entering the root solvers. This improved about 1-2 percentage point.
  • limit the domain of application to the region where the two methods differ more (red and blue region). That doesn't help much either (< 0.5% improvement).

The new extend HI method might just requires a lot of computation. So The following object is added to provide a switch from the simplified to the extended HI method.

HeatIndexAlgorithm,
       \memo Specify whether the simplified or the extended method should be used
       \memo Simplified: based on regression analysis carried out by Lans P. Rothfusz
       \memo Extended: Based on paper by Lu & Romps
       \unique-object
  A1 ; \field Algorithm
       \type choice
       \key Simplified
       \key Extended
       \default Simplified

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING 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

@yujiex yujiex added the Defect Includes code to repair a defect in EnergyPlus label Sep 12, 2024
@yujiex yujiex self-assigned this Sep 12, 2024
@github-actions GitHub Actions
Copy link

⚠️ Regressions detected on macos-14 for commit a71113f

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

@yujiex yujiex added Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus and removed Defect Includes code to repair a defect in EnergyPlus labels Sep 12, 2024
@github-actions GitHub Actions
Copy link

⚠️ Regressions detected on macos-14 for commit 942bb89

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

@github-actions GitHub Actions
Copy link

⚠️ Regressions detected on macos-14 for commit ffd2aa0

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

@github-actions GitHub Actions
Copy link

⚠️ Regressions detected on macos-14 for commit 59b978e

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

@Myoldmopar
Copy link
Member

Even with the latest commit, 4f4809f, it's still causing a 10% increase in instruction count in just our performance test files. We can figure out a solution for this, but it doesn't look like it's going in this release. I'm marking this for next release and we'll deal with this later.

If this method is trimmed as far down as possible but still this much slower, I would suggest adding a switch to calculate heat index using the old way by default and then enabling the new method as an alternative. This would be a user-facing input option, so definitely not in this current release.

@Myoldmopar Myoldmopar added this to the EnergyPlus 25.1 IO Freeze milestone Sep 13, 2024
@yujiex
Copy link
Collaborator Author

yujiex commented Sep 13, 2024 •
edited
Loading

Thanks, @Myoldmopar . It seems that changing the solver type had the biggest impact. Then I tried adding some result caching but those doesn't save as much (like improved 1-2 percentage point). I also tried to limit the domain of its application (in the old PR there was a plot of how results differ between the old and the new methods. I restricted it to only apply on the bottom right region where the two methods differ a lot). That doesn't help much either. Potentially the new method just requires a lot of computation.

(modify): I might have had the equation part wrong for restraining the domain where extended HI is applied (the white region in the following figures. I modified it and tried locally, roughly it improved it by about 6-7%. Might still not be good enough.
image

@github-actions GitHub Actions
Copy link

⚠️ Regressions detected on macos-14 for commit 8808e86

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

@github-actions GitHub Actions
Copy link

⚠️ Regressions detected on macos-14 for commit e3aa916

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

@github-actions GitHub Actions
Copy link

⚠️ Regressions detected on macos-14 for commit efd2514

Regression Summary
  • Table Big Diffs: 1

@github-actions GitHub Actions
Copy link

⚠️ Regressions detected on macos-14 for commit 4d17ac6

Regression Summary
  • Table Big Diffs: 1

@yujiex
Copy link
Collaborator Author

yujiex commented Oct 1, 2024

@Myoldmopar I added an idd object to switch heat index calculation method. Would you mind taking a look? Thanks :)

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 8, 2024

⚠️ Regressions detected on macos-14 for commit dc709f7

Regression Summary
  • Table Big Diffs: 1

@nrel-bot-2
Copy link

@yujiex @Myoldmopar it has been 28 days since this pull request was last updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Myoldmopar Myoldmopar Awaiting requested review from Myoldmopar

Assignees

@yujiex yujiex

Labels
Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus
Projects
None yet
Milestone
EnergyPlus 25.1 IO Freeze
Development

Successfully merging this pull request may close these issues.
6 participants
@yujiex @Myoldmopar @nrel-bot-2 @nrel-bot @nrel-bot-2b @nrel-bot-2c