Skip to content

Conversation

rajeee
Copy link
Contributor

@rajeee rajeee commented Apr 17, 2025

Pull Request Description

Run the upgrades analyzer to generate the options apply logic report for the sdr yaml. These report helps catch and correct apply_logic errors, and also can be used in documentation to explain exactly how many building models the various pieces of the logic applies to.

In addition, it also generates a minimal version of buildstock.csv file. This file contains the minimal number of buildings but with enough diversity such that all of the options in the sdr yaml will apply to at least one building (when possible*).

All these files are commited to project_national/resources/ folder.

*If the logic applies to at least one building in 550K sample.

Related Pull Requests

[related PRs from different repositories]

Related Issues

[What issue(s) is the PR addressing]

Checklist

Required:

Optional (not all items may apply):

@rajeee rajeee requested review from afontani and Copilot April 17, 2025 22:31
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new CI job to run the SDR options analysis and generate a minimal buildstock.csv while ensuring that every option in the sdr YAML applies to at least one building.

  • Adds Git configuration settings for commits triggered by GitHub Actions.
  • Introduces a new workflow job ("sdr_options_analysis") to install required dependencies, generate a 550K sample and minimal buildstock CSV, and commit analysis results.

@afontani
Copy link
Contributor

afontani commented Apr 18, 2025

The new CI workflow. We probably need to make update-results dependent on sdr_options_analysis. I would maybe just call the job sdr_upgrade_analysis.
Screenshot 2025-04-17 at 6 36 04 PM

@afontani
Copy link
Contributor

afontani commented Apr 18, 2025

@rajeee: I guess we could pull out the sdr tests and put them in the new job. I like this new job split, because the integration and analysis tests are more about the baseline and code changes, where the sdr job is about upgrades. These jobs are also parallel to each other which is also good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remind me about the usefulness of this file? The applicability is in sdr_option_application_report.csv file. What additional information is in this file? It is long, and I would like it to be useful if we keep it. Is there another form that this could take that is more readable?

Copy link
Contributor Author

@rajeee rajeee Apr 18, 2025

Choose a reason for hiding this comment

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

This provides a more detailed x-ray into the apply logic. For example, the .csv file in this line tells you that the option applied to 16.7% of the buildings, which can seem correct. But if you look at the detailed report txt file for this option, you will find that that the "Water Heater Efficiency|Electric Heat Pump, 66 gal, 3.35 UEF" logic had no bearing on this logic since there is not water heater with that specification in the 550K sample. This may or may not be correct/expected, but the report file provides you the information to dig further.

Regarding alternative forms, few ideas come to mind.

  1. Split them into one file per upgrade.
    Although the file is large, it is purely text so github seems to be able to handle it okay. When a PR is made with changes/addition to just 1 upgrade, the diff view will only show section relevant to that upgrade (for example the diff after you pulled in the EV DF) so it shouldn't be that verbose in such cases. So, I am not too sure if splitting is worthwhile.

  2. This file would actually be of about the same length as the sdr yaml (minus the comments in the yaml, plus extra filler description in the report txt) if not for a detailed options-interplay-report. This is a more confusing part, and we could maybe remove this, but i think it is useful. For example, from this report, we can tell that 74% of buildings got exactly 3 options applied and 22% got exactly 9 options applied, which serves as a nice verification that options are being applied as intended (like no buildings that got 6 options applied etc).

image

I understand that this report file feels overwhelming, but once we start getting PR with one upgrade at a time, it should be manageable.

Copy link
Contributor

@afontani afontani left a comment

Choose a reason for hiding this comment

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

Overall, I think this will work. Since you are pip installing buildstock-query into the CI environment, this is cleaner than I expected. I added some comments, but we can move forward with the development.

Copy link
Contributor

@afontani afontani left a comment

Choose a reason for hiding this comment

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

Add a changelog entry.

@afontani afontani merged commit 7ad4f1e into develop May 1, 2025
@afontani afontani deleted the sdr_upgrade_analysis branch May 1, 2025 20:04
@joseph-robertson joseph-robertson added this to the ResStock v2025_R1 milestone Sep 4, 2025
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.

3 participants