Skip to content

EUSS/SDR: Demand Flexibility measures combining with PV measure (package feature) #297

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: ccaradon/sdr_2025_r2
Choose a base branch
from
Open

EUSS/SDR: Demand Flexibility measures combining with PV measure (package feature) #297

wants to merge 7 commits into from
+8,920 −57

Conversation

JieXiong9119
Copy link
Collaborator

@JieXiong9119 JieXiong9119 commented Mar 12, 2025 •
edited
Loading

Pull request overview

This PR adds the feature of enabling PV measure within DF lighting measure to create an upgrade package analysis.

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

Author pull request 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 tests for new measures
  • Updated measure .xml(s)
  • Register values added to comstock_column_definitions.csv
  • Both options_lookup.tsv files updated
  • 10k+ test run
  • Change documentation written
  • Measure documentation written
  • ComStock documentation updated
  • Changes reflected in example .yml files
  • Changes reflected in README.md files
  • Added 'See ComStock License' language to first two lines of each code file
  • Implements corresponding measure tests and indexing path in test/reporting_measure_tests.txt, test/workflow_measure_tests.txt, or test/upgrade_measure_tests.txt
  • All new and existing tests pass the CI

Review Checklist

This will not be exhaustively relevant to every PR.

  • Perform a code review on GitHub
  • All related changes have been implemented: data and method additions, changes, tests
  • If fixing a defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • Reviewed change documentation
  • Ensured code files contain License reference
  • Results differences are reasonable
  • Make sure the newly added measures has been added with tests and indexed properly
  • CI status: all tests pass

ComStock Licensing Language - Add to Beginning of Each Code File

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

@JieXiong9119 JieXiong9119 added the upgrade measure PR improves or adds upgrade measures label Mar 12, 2025
@JieXiong9119 JieXiong9119 self-assigned this Mar 12, 2025
@JanghyunJK
Copy link
Contributor

@JieXiong9119 just curious, did you ever try rubocop? https://github.com/NREL/ComStock?tab=readme-ov-file#running-rubocop

@ChristopherCaradonna ChristopherCaradonna added the Do Not Merge Work in progress for code reviews label Mar 12, 2025
@JieXiong9119
Copy link
Collaborator Author

@JieXiong9119 just curious, did you ever try rubocop? https://github.com/NREL/ComStock?tab=readme-ov-file#running-rubocop

No but will try!

@JieXiong9119 JieXiong9119 requested a review from eringold March 12, 2025 20:50
JanghyunJK
JanghyunJK reviewed Mar 17, 2025
View reviewed changes
resources/measures/upgrade_df_lighting_control/resources/call_other_measures.rb
# pass relevant messages/results/variables to parent runner
def child_to_parent_runner_logging(runner_parent, measure_name, results_child, registered_var_list = [])

# Register values from child runner to parent runner
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually added this line too in my latest version:

ComStock/resources/measures/upgrade_hvac_add_heat_pump_rtu/resources/call_other_measures.rb

Line 42 in a761303

unless registered_var_list.empty?

JanghyunJK
JanghyunJK previously approved these changes Mar 17, 2025
View reviewed changes
Copy link
Contributor

@JanghyunJK JanghyunJK left a comment

Choose a reason for hiding this comment

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

@JieXiong9119 could you also check if unit tests are working well for both your measure and PV measure (if there is any) ?

@JieXiong9119 JieXiong9119 force-pushed the jx/demand-flexibility-with-pv branch from f896eea to 1b7d6b4 Compare March 31, 2025 16:11
@JieXiong9119 JieXiong9119 changed the base branch from main to ccaradon/sdr_2025_r2 March 31, 2025 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JanghyunJK JanghyunJK JanghyunJK left review comments

@ChristopherCaradonna ChristopherCaradonna Awaiting requested review from ChristopherCaradonna

@eringold eringold Awaiting requested review from eringold

Assignees

@JieXiong9119 JieXiong9119

Labels
Do Not Merge Work in progress for code reviews Pull Request - Ready for CI upgrade measure PR improves or adds upgrade measures
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JieXiong9119 @JanghyunJK @ChristopherCaradonna