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

Release 2_2_4 patch #442

Merged
merged 8 commits into from
May 1, 2020
Merged

Release 2_2_4 patch #442

merged 8 commits into from
May 1, 2020

Conversation

joseph-robertson
Copy link
Contributor

@joseph-robertson joseph-robertson commented Apr 27, 2020

Pull Request Description

Argument has_hvac_flue for ResidentialAirflow wasn't being assigned when no heating system was sampled.

The proposed solution here is to move the has_hvac_flue argument from the ResidentialAirflow measure out into the HVAC equipment measures (furnace, boiler, unit heater, shared systems). The value assigned to has_hvac_flue will be set as an additional property on the Building model object, and then the airflow measure will parse it from the Building model object. If it doesn't exist, then the value will be set to false.

Checklist

Not all may apply:

  • Unit tests have been added or updated
  • All rake tasks have been run, and pass
  • Documentation has been modified appropriately
  • Any new options are added to project_testing
  • project_testing runs without any failures
  • No unexpected regression test changes
  • All tests are passing (green) on circleci
  • The changelog has been updated appropriately
  • This branch is up-to-date with master

For more information on how to perform these checklist items, see the documentation's Advanced Tutorial.

@joseph-robertson joseph-robertson self-assigned this Apr 27, 2020
@aspeake aspeake linked an issue Apr 28, 2020 that may be closed by this pull request
@joseph-robertson joseph-robertson marked this pull request as ready for review April 28, 2020 14:13
@aspeake
Copy link
Contributor

aspeake commented Apr 28, 2020

@joseph-robertson In addition to Heating Fuel == None and Other Fuel, I am seeing no HVAC systems for homes with all other heating fuels when the following parameters are assigned:

  • HVAC System Is Heat Pump == Yes
  • HVAC System Is Shared == Neither

There is also a small fraction of homes with Heating Fuel == Electricity and HVAC System Is Heat Pump == No that do not get an HVAC system assigned

@JLReyna
Copy link
Member

JLReyna commented Apr 28, 2020

@aspeake this is a mistake in the logic of the distributions (see #434 ). When we fix that issue so that homes assigned heating fuels are appropriately assigned heating systems, the proposed logic of the has_hvac_flue should work. I'm in the process of restructuring all of the HVAC TSVs, but it will probably be a month or so before I put in a PR. Are you wanting to see a quicker fix for the misassignment issue?

HVAC System Heating Fuel Oil Oil Boiler ResidentialHVACBoiler fuel_type=oil "system_type=hot water, forced draft" afue=0.76 oat_reset_enabled=false oat_high=0.0 oat_low=0.0 oat_hwst_high=0.0 oat_hwst_low=0.0 design_temp=180.0 capacity=autosize dse=NA
ResidentialAirflow has_hvac_flue=true
HVAC System Heating Fuel Oil Oil Boiler ResidentialHVACBoiler fuel_type=oil "system_type=hot water, forced draft" afue=0.76 oat_reset_enabled=false oat_high=0.0 oat_low=0.0 oat_hwst_high=0.0 oat_hwst_low=0.0 design_temp=180.0 capacity=autosize dse=NA has_hvac_flue=true
ResidentialAirflow
ResidentialHVACSizing
HVAC System Heating Wood None
HVAC System Heating Wood Wood Stove ResidentialHVACUnitHeater fuel_type=wood efficiency=0.6 fan_power=0 airflow_rate=0 capacity=autosize
Copy link
Contributor

Choose a reason for hiding this comment

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

Move flue argument from airflow to ResidentialHVACUnitHeater?

Copy link
Contributor

Choose a reason for hiding this comment

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

...This is meant to reference the HVAC System Heating Wood option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, missed one. Good catch.

@aspeake aspeake self-requested a review April 28, 2020 20:37
@codecov-io
Copy link

Codecov Report

Merging #442 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #442      +/-   ##
==========================================
- Coverage   93.40%   93.40%   -0.01%     
==========================================
  Files         169      169              
  Lines       49389    49381       -8     
==========================================
- Hits        46132    46124       -8     
  Misses       3257     3257              
Impacted Files Coverage Δ
resources/measures/ResidentialAirflow/measure.rb 99.20% <100.00%> (-0.01%) ⬇️
...sidentialAirflow/tests/residential_airflow_test.rb 100.00% <100.00%> (ø)
...esources/measures/ResidentialHVACBoiler/measure.rb 97.61% <100.00%> (+0.11%) ⬆️
...entialHVACCentralSystemBoilerBaseboards/measure.rb 97.72% <100.00%> (+0.16%) ⬆️
...res/ResidentialHVACCentralSystemFanCoil/measure.rb 98.18% <100.00%> (+0.12%) ⬆️
...asures/ResidentialHVACCentralSystemPTAC/measure.rb 97.36% <100.00%> (+0.22%) ⬆️
...sources/measures/ResidentialHVACFurnace/measure.rb 96.38% <100.00%> (+0.28%) ⬆️
...rces/measures/ResidentialHVACUnitHeater/measure.rb 96.25% <100.00%> (+0.30%) ⬆️
project_singlefamilydetached/tests/test.rb
project_testing/tests/test.rb 93.33% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7616ec9...b3e0810. Read the comment docs.

Copy link
Contributor

@shorowit shorowit left a comment

Choose a reason for hiding this comment

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

Seems like a reasonable change.

@shorowit
Copy link
Contributor

For consistency, the has_water_heater_flue argument could be moved to the appropriate water heater measures.

@joseph-robertson joseph-robertson added this to the ResStock v2.2.4 milestone May 1, 2020
@joseph-robertson joseph-robertson merged commit 9435ce6 into master May 1, 2020
@joseph-robertson joseph-robertson deleted the hvac-flue-patch branch May 1, 2020 18:46
joseph-robertson added a commit that referenced this pull request Jul 8, 2020
33045dedf Update build measure for fixed heater.
cb423405e Merge branch 'master' into build-res-hpxml-v3
63a8b426b Merge pull request #444 from NREL/fixed_space_heater
5fdf842b7 Merge branch 'master' of https://github.com/NREL/OpenStudio-HPXML into fixed_space_heater
2a1c35d72 Merge pull request #443 from NREL/fix_tests_on_windows
df475efa0 Merge branch 'master' of https://github.com/NREL/OpenStudio-HPXML into fix_tests_on_windows
cfd1b0751 Allow FixedHeater (as opposed to PortableHeater) as a HeatingSystemType.
1615cc56a Merge pull request #442 from NREL/adobe_walls
f0e45399c Avoid IO.sysopen for capturing stderr, as it results in the file not being closed on Windows, causing errors when running tests.
684c9bc34 Allow Adobe for WallType.
7dad7d574 Merge pull request #440 from NREL/EPvalidator_bugfix
6a53d5a7f Merge branch 'master' into EPvalidator_bugfix
f915131fb Merge pull request #438 from NREL/error_output
9b7a1e3bd Fix LightingGroup XPath in EPvalidator.rb
0b5c8ea35 Merge branch 'master' into build-res-hpxml-v3
ba6e64c80 Adds more informative output message if OS measure error or E+ error.
b3b3adf0c Merge pull request #437 from NREL/ashrae140_htg_clg
6ba03ab29 Switch to mkdir_p.
5111ab04a More merging of workflow code.
d70c63272 Rename argument.
a64124869 Switches to TMY3 weather for A140 runs. Merges code from hpxml_translator_test.rb and run_simulation.rb into meta_measure.rb.
25b3ba623 Fix test.
679627944 Retrieve HVAC sizing values before runner is reset.
118afa4f2 Small fix to logging.
6b19cbfda Fix ASHRAE 140 test files to only provide heating or cooling with the ideal air system, not both.

git-subtree-dir: resources/hpxml-measures
git-subtree-split: 33045dedfba401e669ebc9e80bf671692bcdc1f4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Argument when No HVAC System is Assigned
5 participants