Skip to content

Conversation

@ChristopherCaradonna
Copy link
Contributor

Pull request overview

Fixes timeseries plotting capabilities for AMI comparisons and upgrade measure comparisons. Includes a new query class that accounts for sample-specific weights from the prior workflow of using BuildStockQuery with building-type-specific weights.

Pull Request Author

This pull request makes changes to (select all the apply):

  • Documentation
  • Infrastructure (includes apptainer image, buildstock batch, dependencies, continuous integration tests)
  • Sampling
  • Workflow Measures
  • Upgrade Measures
  • Reporting Measures
  • Postprocessing

Pull Request Author Checklist:

  • Tagged the pull request with the appropriate label (documentation, infrastructure, sampling, workflow measure, upgrade measure, reporting measure, postprocessing) to help categorize changes in the release notes.
  • Added or edited tests for measures that adequately cover anticipated cases
  • New or changed register values reflected in comstock_column_definitions.csv
  • Both options_lookup.tsv files updated
  • New measure tests add to to test/reporting_measure_tests.txt, test/workflow_measure_tests.txt, or test/upgrade_measure_tests.txt
  • Added 'See ComStock License' language to first two lines of each code file
  • Run rubocop and check log
  • Updated measure .xml(s)
  • Ran 10k+ test run and checked failure rate to make sure no new errors were introduced
  • Measure documentation written or updated
  • ComStock documentation written or updated
  • Change document written and assigned to a reviewer
  • Changes reflected in example .yml files and README.md files

Pull Request Reviewer Checklist:

  • Perform a code review on GitHub
  • All changes have been implemented: data, methods, tests, documentation
  • If fixing a defect, verify by running main branch to reproduce the defect and the PR branch to verify the fix
  • Measure tests written and adequately cover anticipated cases
  • Run measure tests and ensure they pass
  • New measure tests add to to test/reporting_measure_tests.txt, test/workflow_measure_tests.txt, or test/upgrade_measure_tests.txt
  • Ensured code files contain License reference
  • Run rubocop and check log
  • Measure .xml updated
  • CI status: all tests pass
  • ComStock documentation adequately describes the new assumptions
  • Reviewed change documentation, results differences are reasonable, and no new errors introduced
  • Author has addressed comments in change documentation
  • .yml and README.md files updated

ComStock Licensing Language - Add to Beginning of Each Code File

# ComStock™, Copyright (c) 2025 Alliance for Sustainable Energy, LLC. All rights reserved.
# See top level LICENSE.txt file for license terms.

Copy link
Contributor

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 pull request fixes timeseries plotting capabilities for AMI comparisons and upgrade measure comparisons by introducing a new ComStockQueryBuilder class that properly accounts for sample-specific weights. The changes replace the previous workflow that used BuildStockQuery with building-type-specific weights.

Key Changes

  • Introduces a new ComStockQueryBuilder class to generate SQL queries for Athena with proper sample-specific weighting
  • Refactors timeseries plotting methods to use the new query builder and support both state and county-level geographic filtering
  • Updates parameter naming from states to timeseries_locations_to_plot to support both states and counties
  • Adds new methods for creating SightGlass-compatible database tables and views
  • Improves AMI comparison data handling with building type normalization and better DataFrame management

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 56 comments.

Show a summary per file
File Description
postprocessing/setup.py Relaxes Python version requirement from exactly 3.10.12 to >=3.9
postprocessing/comstockpostproc/s3_utilities_mixin.py Comments out fsspec s3 filesystem registration
postprocessing/comstockpostproc/plotting_mixin.py Adds monkey-patching for PyAthena cursor, removes old weighting method, updates timeseries plotting methods to use new query builder, changes column naming from 'baseline' to 'Baseline'
postprocessing/comstockpostproc/comstock_to_ami_comparison.py Adds DataFrame copying, building type normalization, and empty dataframe handling
postprocessing/comstockpostproc/comstock_query_builder.py New file providing SQL query templates for timeseries aggregation, applicability queries, and monthly energy consumption
postprocessing/comstockpostproc/comstock_measure_comparison.py Updates parameter names and method signatures to support flexible geographic locations
postprocessing/comstockpostproc/comstock.py Adds new methods for weighted load profile retrieval, geographic type determination, SightGlass table creation, and AMI timeseries data handling improvements
postprocessing/compare_upgrades.py.template Updates example configuration to use new timeseries_locations_to_plot parameter with state and county examples
postprocessing/compare_upgrades-test_timeseries_plots.py New test file demonstrating timeseries plotting with multiple geographic levels
postprocessing/compare_comstock_to_ami.py.template Updates AMI comparison template with proper workflow and configuration examples

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mdahlhausen mdahlhausen requested a review from rHorsey November 19, 2025 17:05
@NREL NREL deleted a comment from Copilot AI Dec 19, 2025
@NREL NREL deleted a comment from Copilot AI Dec 19, 2025
@NREL NREL deleted a comment from Copilot AI Dec 19, 2025
@NREL NREL deleted a comment from Copilot AI Dec 20, 2025
Copy link
Collaborator

@rHorsey rHorsey left a comment

Choose a reason for hiding this comment

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

Really nice Chris. Some things to clean up (mostly detritus from ai tools I suspect) but really nice work. Happy holidays!

ChristopherCaradonna and others added 6 commits December 22, 2025 07:35
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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