Skip to content
This repository was archived by the owner on Mar 6, 2024. It is now read-only.

Adding effective R-value lookup #160

Merged
merged 73 commits into from
Jan 4, 2022
Merged

Adding effective R-value lookup #160

merged 73 commits into from
Jan 4, 2022

Conversation

nmerket
Copy link
Member

@nmerket nmerket commented Jul 27, 2021

Fixes #156

Pull Request Description

Some HPXML files have an AssemblyRValue element instead of Insulation/Layer.

Checklist

PR Author: Check these when they're done. Not all may apply. strikethrough and check any that do not apply.

PR Reviewer: Verify each has been completed.

  • Code changes (must work)
  • Test exercising your feature or bug fix. Check the coverage report in the build artifacts.
  • All other unit tests passing
  • Update translation docs

@github-actions
Copy link

github-actions bot commented Jul 27, 2021

File Coverage
All files 91%
__init__.py 83%
base.py 93%
exceptions.py 96%
hpxml2.py 86%
hpxml3.py 84%

Minimum allowed coverage is 83%

Generated by 🐒 cobertura-action against e5eaf54

@nmerket
Copy link
Member Author

nmerket commented Jul 27, 2021

Left to do:

  • Roofs
  • Ceilings
  • Floors
  • Knee Walls
  • Foundation Walls (or throw error on AssemblyRValue and fix if someone really needs it)
  • Slabs (or throw error on AssemblyRValue and fix if someone really needs it)

@nmerket
Copy link
Member Author

nmerket commented Jul 27, 2021

Also address the "continuous- exterior" and "continuous - interior" installation types for rigid insulation.

One more wrinkle: in ResStock, for foundation walls we're not specifying insulation material. We may not be checking that for foundation walls, but we should see, but it may be overly restrictive for above grade walls.

@nmerket
Copy link
Member Author

nmerket commented Nov 2, 2021

@bpark1327 to add unit tests and request review.

@bpark1327 bpark1327 marked this pull request as ready for review November 14, 2021 04:33
@bpark1327
Copy link
Collaborator

bpark1327 commented Dec 23, 2021

It seems that the CI fails because of a new test file (i.e., house9.xml) that I added to this branch. house9 is one of the example files generated by ResStock. house9 includes 2 DuctLeakageMeasurement elements -- one for the supply and one for the return ducts. It appears that our workflow is not able to handle it properly. I don't know if it would make sense to calculate the area weighted duct leakage by DuctSurfaceArea. Please advise. @shorowit @nmerket

@shorowit
Copy link
Collaborator

@bpark1327 I think we should support separate supply/return leakage values. Just need to add them together and then it's identical to the existing DuctLeakageMeasurement/DuctLeakage[TotalOrToOutside="to outside"]/Value input.

@bpark1327
Copy link
Collaborator

@bpark1327 I think we should support separate supply/return leakage values. Just need to add them together and then it's identical to the existing DuctLeakageMeasurement/DuctLeakage[TotalOrToOutside="to outside"]/Value input.

Got it. I have a follow-up question. Do we always expect to get leakage values of CFM25? Or, do we need to support the values in other units (e.g., CFM50, Percent (CFM25/sqft), etc.)? @shorowit @nmerket

@shorowit
Copy link
Collaborator

Just supporting CFM25 seems reasonable. We should definitely change DuctLeakageMeasurement/DuctLeakage[TotalOrToOutside="to outside"]/Value to DuctLeakageMeasurement/DuctLeakage[TotalOrToOutside="to outside" and Units="CFM25"]/Value, for example, so that we don't misuse the provided value.

@bpark1327
Copy link
Collaborator

@shorowit Duct blaster measurement will be updated in #183

@bpark1327
Copy link
Collaborator

bpark1327 commented Dec 30, 2021

@nmerket I think it's ready for your review. This PR should wait until #183 gets merged in.

Copy link
Member Author

@nmerket nmerket left a comment

Choose a reason for hiding this comment

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

I think this is looking really good. Just a few notes.

@bpark1327
Copy link
Collaborator

@nmerket I addressed your comments. I moved some functions for walls to hpxml2 and hpxml3.py to be consistent. Please let me know what you think.

Copy link
Member Author

@nmerket nmerket left a comment

Choose a reason for hiding this comment

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

Looks good to me. I think I've put you through enough on this. Let's merge it. I can't approve this because I originally opened the PR. Maybe @yzhou601 could give it a once over?

@nmerket nmerket requested review from yzhou601 and removed request for yzhou601 January 4, 2022 15:50
@nmerket nmerket merged commit 5f69767 into master Jan 4, 2022
@nmerket nmerket deleted the assembly_eff_r_values branch January 4, 2022 20:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle Assembly R-values Incorrectly subtracting R-5 when using ExpandedPolystyreneSheathing element
3 participants