-
Notifications
You must be signed in to change notification settings - Fork 57
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
Add Standard Data Release YAML to GitHub Actions #1329
base: develop
Are you sure you want to change the base?
Conversation
Remove some tests? For example, remove national/testing upgrade ymls in favor of SDR upgrade yml. |
Update changelog Add workflow generator version update number of allowable upgrades Add files to .gitignore update input and output directories Update options lookup arguments
rename sdr yaml file Unzip and upload sdr upgrade artifact Add testing for sdr upgrade run with bsb Reduce upgrade datapoints on national and testing upgrades ymls revert data point changes and reduce sdr datapoints. Remove testing and national upgrades Update process_bsb_analysis Updates. test_bsb_analysis passing Fix test_run_analysis Update testing yml_valid Update expected warnings Remove testing_upgrades from gitignore Update buildstock_missing.csv Make sdr tests an upgrade Add the upgrade osw and xml files as requested from SDR Update the package name load only headers for comparison Add commented out production number of data points remove Zone People Occupant Count: Conditioned Space from timeseries contents New tsvs with updated well pump update changelog update tsvs
Increase n_datapoints Fix option name change from well pump PR Typo reduce datapoints
Update test_upgrade_results Update test_upgrade_columns Fix paths Fix number of expected files Fix run_analysis file path Fix missing upgrade results file in run_analysis results Fix missing upgrade results file in run_analysis results, try 2 Add path errors during artifact upload Cleanup Update the expected number of expected results Switch upgrade to one that applies to all units
args: | ||
# Annual simulation and timestep | ||
build_existing_model: | ||
simulation_control_timestep: 15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to have 15 instead of 60 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to for CI to get a couple more datapoints simulated. 15 min is what we run for the dataset publication.
|
||
# Outputs to include | ||
simulation_output_report: | ||
timeseries_frequency: timestep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be hourly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what we run for the dataset publication. I would like this to be as close as possible to the simulated version. If we forget to change this then we have to re-simulate the entire dataset. Yes, the downside is limiting the number of data points. Can we make the change later if we figure out a solution to the memory limitations?
@@ -0,0 +1,1149 @@ | |||
schema_version: '0.4' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest making the following updates to the baseline yml files for consistency with the new sdr_upgrades_tmy3.yml file:
- order the yml blocks the same (e.g., move
baseline
up) - remove the
eagle
blocks
project_national/national_upgrades/results_csvs/results_up17.csv | ||
project_testing/testing_upgrades/results_csvs/results_up17.csv | ||
name: buildstockbatch_results_csvs | ||
project_national/sdr_upgrades_tmy3/results_csvs/results_up16.csv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the old upgrade yml files, "17" referred to the final "Package Upgrade" upgrade scenario. I believe the thinking here was that the simulation output would be the most comprehensive for testing purposes. I'm not sure that the final "16" has the same significance here for the new yml file.
Also, you may want to avoid hardcoding "16" here if the new yml will expand over time.
@@ -36,3 +36,18 @@ Development Changelog | |||
resstock-estimation: `pull request 436 <https://github.com/NREL/resstock-estimation/pull/436>`_ | |||
|
|||
Assignees: Janet Reyna |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside of scope of this PR, but this heat pump pool heaters entry is missing a .. change::
line above.
@@ -1,7 +1,7 @@ | |||
# frozen_string_literal: true | |||
|
|||
module Constants | |||
NumApplyUpgradeOptions = 25 | |||
NumApplyUpgradeOptions = 100 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires an update to RTD at docs/read_the_docs/advanced_tutorial/increasing_upgrade_options.rst.
@@ -40097,6 +40097,12 @@ Arguments | |||
- Double | |||
- | |||
- The maximum capacity limit applied to the auto-sizing methodology. If not provided, no limit is used. | |||
* - ``heating_system_2_fraction_heat_load_served`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This argument is being added here under "HVAC Secondary Heating Efficiency" because you've added new "Fuel Wall/Floor Furnace" options which assign heating_system_2_fraction_heat_load_served
. But that argument is already assigned in "HVAC Secondary Heating Partial Space Conditioning"...
In the YML file's "simple_filepath" field for utility bill scenario definitions, enter a relative file path to a TSV lookup file (e.g., "data/simple_rates/State.tsv") containing user-defined values corresponding to arguments for fixed costs, marginal rates, and PV. | ||
The first column of the TSV lookup file contains the name of a chosen parameter for which sets of argument values are assigned according to its options. | ||
Any blank fields, or missing options for a parameter not specified in the TSV lookup file, will be defaulted. | ||
|
||
See the Simple section of OpenStudio-HPXML's documentation for [Electricity Rates](https://openstudio-hpxml.readthedocs.io/en/latest/workflow_inputs.html#electricity-rates), and [Fuel Rates](https://openstudio-hpxml.readthedocs.io/en/latest/workflow_inputs.html#fuel-rates), for more information about arguments for fixed charges, marginal rates, and PV. | ||
Refer to BuildStockBatch's documentation for [Residential HPXML Workflow Generator](https://buildstockbatch.readthedocs.io/en/stable/workflow_generators/residential_hpxml.html) for more information about YML file arguments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be copied from resources/data/utility_bills/README.md. Instead, cite the source of the data contained in sdr_rates/State.tsv?
@testing_upgrades = 'project_testing/testing_upgrades' | ||
@national_upgrades = 'project_national/national_upgrades' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete these.
@@ -15,14 +15,13 @@ def before_setup | |||
buildstock_directory = File.join(File.dirname(__FILE__), '..') | |||
|
|||
@testing_baseline = File.join(buildstock_directory, 'testing_baseline') | |||
@national_baseline = File.join(buildstock_directory, 'national_baseline') | |||
@testing_upgrades = File.join(buildstock_directory, 'testing_upgrades') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest deleting this too, but appears its used in some tests below. Suggest a name change so that it's not confused with the "testing project"? Maybe that's overkill; your call.
Pull Request Description
This pull request introduces the Standard Data Release (SDR) YAML file into the "develop" branch of ResStock and into GitHub Actions integration-tests. This PR will give contributors on the SDR measures a place to commit changes and aid in completing an end-to-end workflow in CI.
Workflow changes
.github/workflows/config.yml
. Previously was defaulted to "warn" and wasn't catching that a file was missing. This is done by addingif-no-files-found: error
to theactions/upload-artifact@v4
action. Example:resstock/.github/workflows/config.yml
Lines 121 to 126 in 037a9df
Test Modifications
test/test_analysis_tools.rb
: BuildStockBatch sometimes has some extra columns that are blank. Removing these columns from the test as they are blank.resstock/test/test_analysis_tools.rb
Lines 49 to 54 in 0d7fb07
Area for improved testing
This PR is meant to introduce the SDR project file and do some elementary testing (similar to project national, project testing, and national upgrades). At this point, the tests make sure there is some data and columns. Improving the tests that exist today, building out functional tests, and regression testing are important areas for improvement moving forward. Example: using only 2 datapoints being simulated some upgrades do not get run because the upgrade is invalid for the two units.
Related Pull Requests
None.
Related Issues
Closes #1261 -- "Commit ResStock 2024.2 data release yaml to develop"
Checklist
Required:
Optional (not all items may apply):
openstudio tasks.rb update_measures
has been run