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 Alternate Configurations Build #8696

Merged
merged 3 commits into from
Apr 7, 2021
Merged

Fix Alternate Configurations Build #8696

merged 3 commits into from
Apr 7, 2021

Conversation

mitchute
Copy link
Collaborator

@mitchute mitchute commented Apr 6, 2021

Pull request overview

  • Fixes GH Actions-run alternate build

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

@mitchute mitchute added Defect Includes code to repair a defect in EnergyPlus DoNotPublish Includes changes that shouldn't be reported in the changelog labels Apr 6, 2021
@mitchute mitchute added this to the EnergyPlus Future milestone Apr 6, 2021
@mitchute mitchute self-assigned this Apr 6, 2021
mitchute
mitchute commented Apr 6, 2021
View reviewed changes
Copy link
Collaborator Author

@mitchute mitchute left a comment

Choose a reason for hiding this comment

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

Code walkthrough

.github/workflows/linux_build.yml
-OPENGL_REQUIRED=OFF \
-USE_PSYCH_STATS=ON \
-USE_PSYCH_ERRORS=OFF \
-DOPENGL_REQUIRED=OFF \
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure how this worked before, but this should be the right way to do it.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mitchute without these two USE_PSYCH* lines, is it still exercising the code that depends on those two variables?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@JasonGlazer no, those variables are not used without those CMake flags.

src/EnergyPlus/PsychCacheData.hh
@@ -136,6 +136,12 @@ struct PsychrometricCacheData : BaseGlobalStruct
Array1D<cached_tsat_h_pb> cached_Tsat_HPb; // DIMENSION(0:tsat_hbp_cache_size)
#endif

#ifdef EP_psych_stats
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving these over to PsychCacheData to avoid circular references

src/EnergyPlus/Psychrometrics.cc
@@ -712,7 +708,7 @@ namespace Psychrometrics {

Real64 PsyPsatFnTemp(EnergyPlusData &state,
Real64 const T, // dry-bulb temperature {C}
std::string const &CalledFrom // routine this function was called from (error messages)
[[maybe_unused]] std::string const &CalledFrom // routine this function was called from (error messages)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Silence warning

src/EnergyPlus/PsychCacheData.hh Outdated
@@ -136,6 +136,12 @@ struct PsychrometricCacheData : BaseGlobalStruct
Array1D<cached_tsat_h_pb> cached_Tsat_HPb; // DIMENSION(0:tsat_hbp_cache_size)
#endif

#ifdef EP_psych_stats
// EnergyPlus::Psychrometrics::NumPsychMonitors = 19
Array1D<Int64> NumTimesCalled = Array1D<Int64>(19, 0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Myoldmopar this could use a review. I tried including Psychrometrics.hh in this file so I could create the array using the reference to NumPsychMonitors, but there must still be something else that is needed. I just hardcoded the number for now to get it going, but I'm happy to change it if you can point me in the right direction.

@mitchute mitchute requested a review from Myoldmopar April 6, 2021 16:25
@JasonGlazer
Copy link
Contributor

@mitchute I was just struggling with trying to figure this out yesterday. Thanks so much!

@mitchute
Copy link
Collaborator Author

mitchute commented Apr 7, 2021

The CI problems are unrelated to this work. Merging so we can at least get this issue cleaned up on other PRs.

@mitchute mitchute merged commit 29b5dec into develop Apr 7, 2021
@mitchute mitchute deleted the alt_build_options branch April 7, 2021 03:44
JasonGlazer added a commit that referenced this pull request Apr 7, 2021
@JasonGlazer
Last change from #8696
10ecaf7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JasonGlazer JasonGlazer JasonGlazer left review comments

@Myoldmopar Myoldmopar Awaiting requested review from Myoldmopar

Assignees

@mitchute mitchute

Labels
Defect Includes code to repair a defect in EnergyPlus DoNotPublish Includes changes that shouldn't be reported in the changelog
Projects
None yet
Milestone
EnergyPlus 9.6 Release
Development

Successfully merging this pull request may close these issues.
7 participants
@mitchute @JasonGlazer @nrel-bot @nrel-bot-2 @nrel-bot-3 @nrel-bot-2b @nrel-bot-2c