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.

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

EUSS FY25 Release 2 - Electric Resistance Boilers #255

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

EUSS FY25 Release 2 - Electric Resistance Boilers #255

wants to merge 9 commits into from
+71,134 −0

Conversation

mpraprost
Copy link
Contributor

Pull request overview

Measure code review for electric boiler measure. Do not merge yet until EUSS merge.

  • Fixes #ISSUENUMBERHERE (IF RELEVANT)

Pull Request Author: Marley

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.

eringold
eringold requested changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@eringold eringold left a comment

Choose a reason for hiding this comment

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

a couple test changes, file cleanup and some merge conflicts to resolve

postprocessing/compare_upgrades.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

postprocessing/compare_runs.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

resources/measures/upgrade_hvac_electric_boiler/tests/elec_boiler_test.rb
Comment on lines +234 to +235
assert(boiler_fuel_type = "Electricity")
assert(boiler_efficiency = 1.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

this asserts assignment ('='), not equality ('=='). How about e.g.: assert_equal('Electricity', boiler.fuelType)

resources/measures/upgrade_hvac_electric_boiler/tests/elec_boiler_test.rb
Comment on lines +279 to +280
assert(boiler_fuel_type = "Electricity")
assert(boiler_efficiency = 1.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eringold eringold eringold requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@eringold eringold

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mpraprost @eringold