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

Update the DOE commercial prototype building models test files #8686

Merged
merged 9 commits into from
Sep 9, 2021

Conversation

lymereJ
Copy link
Collaborator

@lymereJ lymereJ commented Mar 31, 2021

Pull request overview

Update the DOE commercial prototype building models test files (based on 90.1-2019 instead of 90.1-2016).

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

Reviewer

This will not be exhaustively relevant to every PR.

  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • 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

@Myoldmopar Myoldmopar added the NewFeature Includes code to add a new feature to EnergyPlus label Apr 29, 2021
@Myoldmopar
Copy link
Member

This looks like a clean set of changes to the test files, not even any diffs. The CI results would be all clean if develop was pulled in. I am partially inclined to not waste those cycles, but I'd like to leave the door open for @JasonGlazer or @mjwitte or anyone else to take a deeper dive into the file changes if interested, so I'll go ahead and get fresh results anyway.

@Myoldmopar
Copy link
Member

(There are some Github Actions failures over in the fork where this branch actually lives that aren't showing up on the PR. I think the errors are related to the repo name changing, but I could be wrong. Anyway, they aren't significant here.)

@Myoldmopar
Copy link
Member

@lymereJ 2 of the new/modified input files are timing out in our debug build. This most likely indicates there is something wrong in the file (too many max hvac iterations or something). We need to figure out how to either simplify some aspect of the file or clean up runtime issues if there are any. We cannot have our test files taking multiple hours to run for every pushed commit of changes. Let me know if you want to discuss.

@lymereJ
Copy link
Collaborator Author

lymereJ commented Apr 30, 2021

@Myoldmopar - Really sorry about that, we did run them locally and most completed under 30 min (mostly between 0-10 min). Are you able to point out which one is timing out? I suspect that it is the high rise apartment model as it took 53 min to run on my machine.

@nrel-bot-3
Copy link

@lymereJ @lgentile it has been 28 days since this pull request was last updated.

lymereJ and others added 3 commits July 6, 2021 15:24
… folder. (2) three weather files are added (3) Version of all prototoype models is 9.6 (4) ApartmentHighRise and OutPatientHealthCare prototypes are modified to reduce the runtime
…en Fan:ZoneExhaust objects and DesignSpecification:OutdoorAir got resolved
@NREL NREL deleted a comment from nrel-bot-2b Aug 11, 2021
@NREL NREL deleted a comment from nrel-bot-3 Aug 11, 2021
@mitchute
Copy link
Collaborator

mitchute commented Sep 2, 2021

What's the status here? Are you still targeting this release? Did the timing issues get resolved?

@lymereJ
Copy link
Collaborator Author

lymereJ commented Sep 2, 2021

@mitchute - We are waiting for review. Yes, this release would be great. I think that the last CI results show that updated files that were timing out aren't anymore.

@mitchute
Copy link
Collaborator

mitchute commented Sep 2, 2021

@lymereJ that's fine. Can you resolve these conflicts so we can get a fresh CI run? Let me know if you need help with that.

@rraustad
Copy link
Contributor

rraustad commented Sep 4, 2021

So what was changed here? Wall R, equip efficiency, etc. I can't see anything on the Git diffs. I get the 2016 -> 2019 updates but can we get a summary of changes, a description, etc. I don't see a related issue so there is nothing to reference, except 90.1 and I'd still like to see a summary for these changes as documentation.

add_simulation_test(IDF_FILE ASHRAE9012016_SchoolPrimary_Denver.idf EPW_FILE USA_CO_Golden-NREL.724666_TMY3.epw)
add_simulation_test(IDF_FILE ASHRAE9012016_SchoolSecondary_Denver.idf EPW_FILE USA_CO_Golden-NREL.724666_TMY3.epw)
add_simulation_test(IDF_FILE ASHRAE9012016_Warehouse_Denver.idf EPW_FILE USA_CO_Golden-NREL.724666_TMY3.epw)
# ASHRAE 90.1-2019 DOE commercial prototype models
Copy link
Contributor

Choose a reason for hiding this comment

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

So we are deleting the 90.1 2016 prototypes from this repo. That's OK, but are these files saved somewhere for future use? A DOE web site?

Choose a reason for hiding this comment

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

All versions of the 90.1 prototype models are saved at the DOE's Building Energy Codes Program website in the zip files.

@jiangithub
Copy link

So what was changed here? Wall R, equip efficiency, etc. I can't see anything on the Git diffs. I get the 2016 -> 2019 updates but can we get a summary of changes, a description, etc. I don't see a related issue so there is nothing to reference, except 90.1 and I'd still like to see a summary for these changes as documentation.

The technical support document Energy Savings Analysis: ANSI/ASHRAE/IES Standard 90.1-2019 (PDF page 49, Appendix B: Modeling of Individual Addenda) provides a summary of changes from the 90.1-2016 prototypes to 90.1-2019.

@rraustad
Copy link
Contributor

rraustad commented Sep 8, 2021

We are just going to have to take this on faith as I cannot easily compare differences. At least it was the authors of the DOE publication that made these changes, that is as good as one would expect. I pulled this branch and even the warehouse is so different (using WinDiff) that it would take a hand review of each file to ensure correct data.

Other than a new weather file and replacement of existing example files and updating of the testfiles\CMakeLists.txt. I do not believe there is anything else to update. @Myoldmopar does the new weather file need to be documented anywhere in some file?

@Myoldmopar
Copy link
Member

I concur about the contents of the files, it's as reliable an author as we could expect. CI is happy, with Windows failure being an anomaly. The weather file is fine, and we aren't packaging it up anyway, it's just in our tests. I will merge this in. Thanks @lymereJ and @rraustad.

@Myoldmopar Myoldmopar merged commit 7e651f8 into NREL:develop Sep 9, 2021
@lymereJ
Copy link
Collaborator Author

lymereJ commented Sep 9, 2021

Thanks to @jungwyoungs who did most of it!

@jungwyoungs
Copy link
Collaborator

Thanks all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NewFeature Includes code to add a new feature to EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.