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

Addresses #4647, wrap the E+ Table:Lookup, Table:IndependentVariableList, and Table:IndependentVariable objects #173

Merged
merged 7 commits into from
Sep 8, 2022

Conversation

jmarrec
Copy link
Contributor

@jmarrec jmarrec commented Sep 1, 2022

Pull request overview

Adjust tests to use the TableLookup or TableMultivariableLookup, conditionally based on the version of the SDK

Link to relevant GitHub Issue(s) if appropriate: NREL/OpenStudio#4652

Link to the Linux.deb installer to use for CI Testing. If not set, it will default to latest official release.
[OpenStudio Installer]: http://openstudio-ci-builds.s3-website-us-west-2.amazonaws.com/PR-4652/OpenStudio-3.4.1-alpha%2B45452eabd2-Ubuntu-18.04.deb

This Pull Request is concerning:

  • Case 1 - NewTest: a new test for a new model API class,
  • Case 2 - TestFix: a fix for an existing test. The GitHub issue should be referenced in the PR description
  • Case 3 - NewTestForExisting: a new test for an already-existing model API class
  • Case 4 - Other: Something else, like maintenance of the repo, or just committing test results with a new OpenStudio version.

Case 2: Fix for an existing test

Please include a link to the specific test you are modifying, and a description of the changes you have made and why they are required.

Work Checklist

The change:

  • affects site kBTU results
  • does not affect total site kBTU results

If it affects total site kBTU:

  • Test has been run backwards (see Instructions for Running Docker) for all OpenStudio versions to update numbers
  • Changes did not make the test fail in older OpenStudio versions where it used to pass
  • Matching OSM has been replaced with the output of the ruby test for the oldest OpenStudio release where it passes.
  • All new/changed out.osw have been committed for official OpenStudio versions only

Either way:

  • Ruby test is still stable: when run multiple times on the same machine, it produces the same total site kBTU.

    • I ensured that I assign systems/loads/etc in a repeatable manner (eg: if I assign Terminals to thermalZones, I do model.getThermalZones.sort_by{|z| z.name.to_s}.each do ... so I am sure I put the same ZoneHVAC systems to the same zones regardless of their order)
    • I tested stability using process_results.py (see python process_results.py --help for usage).
      Please paste the text output or heatmap png generated after running the following commands:
      # Clean up all custom-tagged OSWs
      python process_results.py test-stability clean
      # Run your test 5 times in a row. Replace `testname_rb` (eg `airterminal_fourpipebeam_rb`)
      python process_results.py test-stability run -n testname_rb
      # Check that they all passed
      python process_results.py test-status --tagged
      # Check site kBTU differences
      python process_results.py heatmap --tagged
      
  • If relevant, new fields that can be autosized have been added to autosize_hvac.rb to ensure the autosizedXXX values methods do work


Review Checklist

  • Code style (indentation, variable names, strip trailing spaces)
  • Functional code review (it has to work!)
  • Matching OSM test has been added or # TODO added to model_tests.rb
  • Appropriate out.osw have been committed
  • Test is stable
  • Object is tested in autosize_hvac as appropriate
  • The appropriate labels have been added to this PR:
    • One of: NewTest, TestFix, NewTestForExisting, Other
    • If NewTest: add PendingOSM or AddedOSM

@jmarrec jmarrec added the TestFix PR Type: a fix for an existing test label Sep 1, 2022
@jmarrec jmarrec self-assigned this Sep 1, 2022
jmarrec added a commit to NREL/OpenStudio that referenced this pull request Sep 1, 2022
jmarrec added a commit to NREL/OpenStudio that referenced this pull request Sep 1, 2022
github-rubocop-actions[bot] and others added 5 commits September 1, 2022 08:33
Copy link
Contributor

@joseph-robertson joseph-robertson left a comment

Choose a reason for hiding this comment

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

Have we verified equivalent results across the Gem::Version.new('3.4.0') switch?

model/simulationtests/tablelookup.rb Show resolved Hide resolved
@jmarrec jmarrec merged commit 8030b89 into develop Sep 8, 2022
@jmarrec jmarrec deleted the table-lookup branch September 8, 2022 08:18
@jmarrec jmarrec added NewTest PR type: a new test for a new model API class PendingOSM A matching OSM test has yet to be added with the next official OpenStudio Release labels Jan 10, 2023
jmarrec added a commit that referenced this pull request Jan 10, 2023
@jmarrec jmarrec added AddedOSM A matching OSM test has been added with an official OpenStudio release and removed PendingOSM A matching OSM test has yet to be added with the next official OpenStudio Release labels Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AddedOSM A matching OSM test has been added with an official OpenStudio release NewTest PR type: a new test for a new model API class TestFix PR Type: a fix for an existing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants