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

Fix: All buildings have double-loaded corridors #522

Merged
merged 10 commits into from
Jan 7, 2021

Conversation

afontani
Copy link
Contributor

@afontani afontani commented Dec 16, 2020

Pull Request Description

Problem

In the project_national/housing_characteristics/Corridor.tsv, all buildings are assigned a "Double-Loaded Corridor." However, the single-family detached, mobile homes, and single-family attached buildings do not have corridors. For single-family detached, mobile homes, and single-family attached buildings, measures/BuildExistingModel/measure.rb removes the calls to the ResidentialGeometryCreateMultifamily measure.

This workflow creates a column in the buildstock.csv and results.csv that says all buildings have a double-loaded corridor.

Fix

For transparency, a new option is introduced "Option=Not Applicable" where all single-family detached, mobile-homes, and single-family detached are assigned. All multi-family buildings still have a double-loaded corridor. This change makes the Corridor column in the buildstock.csv and results.csv a little more informative and transparent.

There are no changes to the simulated models, therefore there are no energy impacts of this change.

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.

…RECS dependency

In the buildstock.csv and results.csv all buildings have a double-loaded corridor. This change has no energy impacts, but is more transparent about which buildings have corridors.
@joseph-robertson
Copy link
Contributor

@afontani Do you want to try a quick test run to make sure everything shows up as expected in the buildstock/results csv?

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.

Do we need a companion PR on resstock-estimation for these tsv changes?

@afontani
Copy link
Contributor Author

@joseph-robertson: So I ran some tests and saw that there are some reproducibility issues with our simulations corresponding to some measures, but this PR should not be the reason for the reproducibility issue.

In terms of resstock-estimation, This housing characteristic is not scripted and does not exist in the resstock-estimation repo. So I don't think we need a companion PR.

@joseph-robertson
Copy link
Contributor

@afontani I'm sort of confused why integrity checks still pass. Aren't we now lacking assignment of corridor_position and corridor_width for the ResidentialGeometryCreateMultifamily measure? Even when the sample is single-family detached, the ResidentialGeometryCreateMultifamily measure is referenced by, e.g., Bedrooms.

@afontani afontani merged commit b8be4d1 into develop Jan 7, 2021
@afontani afontani deleted the bugfix/corridors_options branch January 7, 2021 17:18
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.

2 participants