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 #432 #447

Merged
merged 3 commits into from
Jan 15, 2025
Merged

Fix #432 #447

merged 3 commits into from
Jan 15, 2025

Conversation

jtlangevin
Copy link
Collaborator

  • Handles use of "pool heaters and pumps" in savings shape CSV (the two are separated in the baseline msegs and load data).
  • Restricts ice storage measure to large office, the only building type for which a savings shape was simulated.
  • Changes warning about missing savings shape data to be conditional on the verbose user option being true.
  • Adds tracking of those warnings so that a given region/system, building type, and end use combination will not generate warnings about a missing shape more than once in verbose mode.

@@ -5900,7 +5905,7 @@ def fill_mkts(self, msegs, msegs_cpl, convert_data, tsv_data_init, opts,
# Pull TSV scaling fractions and shapes
tsv_scale_fracs, tsv_shapes = self.gen_tsv_facts(
tsv_data, mskeys, bldg_sect, convert_data, opts,
cost_energy_meas)
cost_energy_meas, save_shp_warn)
Copy link
Collaborator

Choose a reason for hiding this comment

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

save_shp_warn appears to always be an empty list here, why initialize it above and why pass it in this method?

@@ -6547,7 +6553,7 @@ def gen_tsv_facts(
updated_tsv_fracs, updated_tsv_shapes = self.apply_tsv(
load_fact, ash_czone_wts, eplus_bldg_wts, cost_fact_hourly,
carbon_fact_hourly, mskeys, bldg_sect, eu, opts, cost_yr_map,
carb_yr_map)
carb_yr_map, save_shp_warn)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar comment to above, this looks like it will always be passed as an empty list.

Comment on lines 6582 to 6584
def apply_tsv(self, load_fact, ash_cz_wts, eplus_bldg_wts,
cost_fact_hourly, carbon_fact_hourly, mskeys, bldg_sect,
eu, opts, cost_yr_map, carb_yr_map):
eu, opts, cost_yr_map, carb_yr_map, save_shp_warn):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would make sense to initialize save_shp_warn in this method instead of having as an argument, as it does not appear to be returned or updated anywhere outside of apply_tsv.

Initializing as an empty list would work before for bldg in eplus_bldg_wts.keys():

Copy link
Collaborator Author

@jtlangevin jtlangevin Dec 2, 2024

Choose a reason for hiding this comment

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

That list stores previous warnings generated within apply_tsv to avoid duplication of warning messages. (The list is appended to here.) If it's re-initialized each time apply_tsv is run, that history of warning messages would be removed.

Copy link
Collaborator Author

@jtlangevin jtlangevin Dec 2, 2024

Choose a reason for hiding this comment

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

@aspeake Note updated approach per our discussion in commit dae260a

@jtlangevin
Copy link
Collaborator Author

compare_bugfix_save-warns.pptx

Checking the differences in the CI vs. master, the change it results looks reasonable.

I limited the chillers measure baseline market to just large office, so its baseline (thick red) went down while savings look to be about the same (more visible with new y axis scaling), against as I'd expect since large office was the only savings shape assessed for that measure.

For pool pumps, the baseline stays the same but savings increase as my changes applied the relative savings shapes from pool heaters and pumps in CSV to each of pool heaters and pool pumps (separate end uses in Scout). Before none of those savings were being pulled in.

Copy link
Collaborator

@aspeake aspeake 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. Do you think we should update our integration test config file to include the --verbose flag? (https://github.com/trynthink/scout/blob/master/tests/integration_testing/integration_test.yml)

@jtlangevin
Copy link
Collaborator Author

Looks good. Do you think we should update our integration test config file to include the --verbose flag? (https://github.com/trynthink/scout/blob/master/tests/integration_testing/integration_test.yml)

We could, although I think the end result will be lots of other messages being spit out (including, e.g., notifications of cost conversions or missing segments) that aren't very necessary or helpful – changes in the overall results should be sufficient to flag issues. So I'd vote for keeping without verbose.

@jtlangevin jtlangevin removed the request for review from trynthink December 9, 2024 15:49
jtlangevin and others added 3 commits December 9, 2024 10:51
Savings shape warnings are now stored within the measure handyvars attribute to retain warning history across multiple microsegment loops.
@jtlangevin jtlangevin added this to the v1.0.1 milestone Dec 9, 2024
@jtlangevin jtlangevin requested a review from trynthink December 11, 2024 21:05
@aspeake aspeake merged commit 802ae93 into master Jan 15, 2025
9 checks passed
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