Open
Conversation
It act in a more functional programming sort of way, with functions like `write_osw` and `write_osws` returning a osw(s), but not setting @workflow
There was a problem hiding this comment.
Pull request overview
This PR updates the BuildingSync gem for newer OpenStudio / Ruby tooling by refactoring workflow generation/execution (baseline + report OSWs), introducing a new BCL weather-file downloader, and cleaning up/streamlining the test suite and fixtures.
Changes:
- Refactors translator/workflow flow to support writing/running a baseline OSW and then writing/running report OSWs.
- Replaces legacy BCL weather download logic and adds new OSW argument population utilities / baseline OSW template.
- Removes a large amount of legacy model-articulation code, specs, and XML fixtures; updates licensing headers and CI/runtime dependency versions.
Reviewed changes
Copilot reviewed 110 out of 130 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/tests/xml_get_set_spec.rb | License header update |
| spec/tests/workflow_maker_spec.rb | License header update + whitespace cleanup |
| spec/tests/utility_spec.rb | License header update |
| spec/tests/translator_write_osw_spec.rb | Adds baseline/report OSW integration spec |
| spec/tests/translator_spec.rb | Removes legacy translator integration spec |
| spec/tests/translator_scenario_simulations_spec.rb | License header update |
| spec/tests/translator_scenario_generation_spec.rb | License header update |
| spec/tests/time_series_spec.rb | License header update |
| spec/tests/selection_tool_spec.rb | License header update |
| spec/tests/scenario_spec.rb | License header update |
| spec/tests/resource_use_spec.rb | License header update |
| spec/tests/report_spec.rb | License header update |
| spec/tests/model_articulation_test/weather_file_download_spec.rb | Switches weather download test to new downloader |
| spec/tests/model_articulation_test/site_spec.rb | Removes IDF comparison integration test |
| spec/tests/model_articulation_test/service_hot_water_system_spec.rb | Removes legacy spec |
| spec/tests/model_articulation_test/occupancy_types_spec.rb | License header update |
| spec/tests/model_articulation_test/loads_system_spec.rb | Removes legacy spec |
| spec/tests/model_articulation_test/lighting_system_type_spec.rb | Removes legacy spec |
| spec/tests/model_articulation_test/hvac_system_spec.rb | Removes legacy spec |
| spec/tests/model_articulation_test/hospital_occupancy_type_spec.rb | License header update |
| spec/tests/model_articulation_test/facility_spec.rb | Removes legacy integration-heavy spec blocks |
| spec/tests/model_articulation_test/envelope_system_spec.rb | Removes legacy spec |
| spec/tests/model_articulation_test/building_spec.rb | Removes climate-zone setter specs |
| spec/tests/model_articulation_test/building_section_spec.rb | License header update |
| spec/tests/helper_spec.rb | License header update |
| spec/tests/generator_spec.rb | License header update |
| spec/tests/epw_test_spec.rb | License header update |
| spec/tests/constants_spec.rb | Removes check for deleted phase-zero base OSW file |
| spec/tests/building_sync_spec.rb | License header update |
| spec/tests/all_resource_total_spec.rb | License header update |
| spec/spec_helper.rb | Removes sizing-run helper functions tied to deleted tests |
| spec/files/v2.4.0/L000_OpenStudio_Pre-Simulation_04.xml | Removes fixture |
| spec/files/v2.4.0/L000_OpenStudio_Pre-Simulation_03.xml | Removes fixture |
| spec/files/v2.4.0/L000_OpenStudio_Pre-Simulation_02.xml | Removes fixture |
| spec/files/v2.4.0/L000_OpenStudio_Pre-Simulation_01.xml | Removes fixture |
| spec/files/v2.4.0/Golden Test File.xml | Updates example provenance + version/schemaLocation/IDs |
| spec/files/v2.4.0/DC GSA HeadquartersWithClimateZone.xml | Removes fixture |
| spec/files/v2.4.0/DC GSA Headquarters.xml | Removes fixture |
| spec/files/v2.4.0/building_151_one_scenario.xml | Removes fixture |
| spec/files/v2.4.0/building_151_no_measures.xml | Removes fixture |
| README.md | Rebrands docs to “BOSS” and updates usage/testing guidance |
| Rakefile | License header update |
| osw_test/test.osw | Removes legacy OSW test artifact |
| LICENSE.md | Updates copyright years/holder |
| lib/measures/building_sync_to_openstudio/measure.rb | Adds license header + minor whitespace cleanup |
| lib/measures/building_sync_to_openstudio/LICENSE.md | Updates copyright holder |
| lib/buildingsync/version.rb | License header update |
| lib/buildingsync/utility.rb | License header update |
| lib/buildingsync/translator.rb | Refactors translator initialization + adds baseline/report OSW helpers |
| lib/buildingsync/time_series.rb | License header update |
| lib/buildingsync/selection_tool.rb | License header update |
| lib/buildingsync/scenario.rb | License header update |
| lib/buildingsync/resource_use.rb | License header update |
| lib/buildingsync/report.rb | License header update |
| lib/buildingsync/model_articulation/wall_system_type.rb | Removes legacy class |
| lib/buildingsync/model_articulation/systems_map.rb | Adds HVAC system-type mapping table |
| lib/buildingsync/model_articulation/spatial_element.rb | Adds standard context to initialization; updates building/system type selection logic |
| lib/buildingsync/model_articulation/site.rb | Updates API to set standard template + weather/CZ without generating OSM directly |
| lib/buildingsync/model_articulation/service_hot_water_system.rb | Removes legacy class |
| lib/buildingsync/model_articulation/roof_system_type.rb | Removes legacy class |
| lib/buildingsync/model_articulation/measure.rb | License header update |
| lib/buildingsync/model_articulation/location_element.rb | Changes climate-zone formatting + standard selection logic |
| lib/buildingsync/model_articulation/loads_system.rb | Removes legacy class |
| lib/buildingsync/model_articulation/lighting_system.rb | Removes legacy class |
| lib/buildingsync/model_articulation/foundation_system_type.rb | Removes legacy class |
| lib/buildingsync/model_articulation/exterior_floor_system_type.rb | Removes legacy class |
| lib/buildingsync/model_articulation/envelope_system.rb | Removes legacy class |
| lib/buildingsync/model_articulation/DOE_to_DEER_building_type.rb | Adds DOE→DEER building-type mapping table |
| lib/buildingsync/model_articulation/building_system.rb | Removes legacy base class |
| lib/buildingsync/model_articulation/building_section.rb | Adds floor-to-floor height + occupancy fallback handling |
| lib/buildingsync/makers/workflow_maker_base.rb | Removes unused workflow mutation helpers; keeps argument setter |
| lib/buildingsync/makers/phase_zero_base.osw | Removes legacy base OSW template |
| lib/buildingsync/makers/osw_arg_populator.rb | Adds baseline OSW argument population helpers |
| lib/buildingsync/makers/empty_baseline.osw | Adds empty baseline OSW template |
| lib/buildingsync/helpers/xml_get_set.rb | License header update |
| lib/buildingsync/helpers/Model.hvac.rb | Removes legacy OpenStudio::Model monkey patch |
| lib/buildingsync/get_bcl_weather_file.rb | Removes legacy BCL weather downloader |
| lib/buildingsync/generator.rb | License header update |
| lib/buildingsync/extension.rb | License header update |
| lib/buildingsync/contact.rb | License header update |
| lib/buildingsync/constants.rb | Adds empty-baseline OSW path constant |
| lib/buildingsync/bcl_weather_file_downloader.rb | Adds new BCL+S3-based weather downloader implementation |
| lib/buildingsync/audit_date.rb | License header update |
| lib/buildingsync/all_resource_total.rb | License header update |
| lib/buildingsync.rb | License header update |
| Gemfile | Simplifies local gem overrides to commented examples |
| doc_templates/LICENSE.md | Updates copyright holder |
| doc_templates/copyright_ruby.txt | Updates copyright holder |
| doc_templates/copyright_js.txt | Updates copyright holder |
| doc_templates/copyright_erb.txt | Updates copyright holder |
| buildingsync.gemspec | Updates Ruby requirement and dependency versions; adds httparty + rubocop |
| .gitignore | Ignores entire osw_test directory |
| .github/workflows/deploy.yml | Updates Ruby version to 3.2.2 |
| .github/workflows/continuous_integration.yml | Updates container tags (3.9.0, and 9.0 for model-articulation job) |
Comments suppressed due to low confidence (2)
lib/buildingsync/model_articulation/location_element.rb:53
determine_climate_zonelogs referencestandard_to_be_used, but the method no longer takes that parameter. As written, hitting the warning branches will raise aNameErrorand break execution. Use@standard_to_be_used(or reintroduce the parameter) in the log message interpolation.
lib/buildingsync/model_articulation/spatial_element.rb:62read_floor_areasnow leaves@total_floor_area(Gross) unconverted, while other floor area types are still converted from ft^2 to m^2. This creates inconsistent units within the same object and will skew downstream calculations (eg. total vs conditioned areas, and UDFs labeled "(m^2)"). Either keep Gross in the same units as the others (convert to m^2) or stop converting the other floor areas so everything is consistent.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
spec/tests/model_articulation_test/weather_file_download_spec.rb
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 111 out of 131 changed files in this pull request and generated 18 comments.
Comments suppressed due to low confidence (2)
lib/buildingsync/model_articulation/location_element.rb:52
determine_climate_zoneinterpolatesstandard_to_be_usedin the log message, but that local variable no longer exists after the refactor (method now uses@standard_to_be_used). This will raise aNameErrorthe first time the warning branch executes. Replace the interpolation with@standard_to_be_used(or reintroduce a local) in both warning messages.
lib/buildingsync/model_articulation/spatial_element.rb:58read_floor_areasno longer converts the Gross floor area from ft^2 to m^2, but other floor areas are still converted and later UDFs are labeledTotalFloorArea(m^2). This creates inconsistent units (and likely incorrect geometry/measure inputs). Either convert Gross to m^2 like the other areas, or update downstream expectations and the UDF labels to match the chosen unit system consistently.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
7
to
+16
| require 'buildingsync/report' | ||
| require 'buildingsync/contact' | ||
| require 'buildingsync/helpers/helper' | ||
| require 'buildingsync/helpers/xml_get_set' | ||
| require 'buildingsync/helpers/Model.hvac' | ||
| require 'pry' | ||
|
|
||
| require_relative 'site' | ||
| require_relative 'loads_system' | ||
| require_relative 'envelope_system' | ||
| require_relative 'hvac_system' | ||
| require_relative 'lighting_system' | ||
| require_relative 'service_hot_water_system' | ||
| require_relative 'measure' | ||
| require_relative 'systems_map' | ||
|
|
There was a problem hiding this comment.
Facility requires pry at runtime. pry is declared as a development dependency in the gemspec, so consumers installing the gem without development deps will hit LoadError. Remove this require or guard it behind a development/debug flag.
Comment on lines
+144
to
158
| # get sum of /Systems/LightingSystems/PlugLoads/WeightedAverageLoad | ||
| # @return [String] | ||
| def get_total_weighted_average_load | ||
| # if no plug_loads, return nil | ||
| plug_loads = @base_xml.elements["#{@ns}:Systems/#{@ns}:PlugLoads/"] | ||
| return nil if plug_loads.nil? | ||
|
|
||
| # get all weighted_average_loads | ||
| plug_loads = plug_loads.reject {|s| s.class == REXML::Comment} | ||
| all_weighted_average_loads = plug_loads.map {|s| s.elements["#{@ns}:WeightedAverageLoad/"]} | ||
|
|
||
| # if any nil, return nil, else return sum | ||
| return nil if !all_weighted_average_loads.all? | ||
| return all_weighted_average_loads.map {|s| s.first.to_s.to_f}.sum | ||
| end |
There was a problem hiding this comment.
get_total_weighted_average_load uses @base_xml.elements[".../PlugLoads/"], which returns a single element (or nil), not a collection. Calling reject/map on it will raise. Use get_elements (to return an array) and then traverse each PlugLoad element to read its WeightedAverageLoad value text.
Comment on lines
+160
to
174
| # get sum of /Systems/LightingSystems/LightingSystem/InstalledPower | ||
| # @return [String] | ||
| def get_total_installed_power | ||
| # if no lighting systems, return nil | ||
| lighting_systems = @base_xml.elements["#{@ns}:Systems/#{@ns}:LightingSystems/"] | ||
| return nil if lighting_systems.nil? | ||
|
|
||
| # get all all_installed_powers | ||
| lighting_systems = lighting_systems.reject {|s| s.class == REXML::Comment} | ||
| all_installed_powers = lighting_systems.map {|s| s.elements["#{@ns}:InstalledPower/"]&.first&.to_s&.to_f} | ||
|
|
||
| # if any nil, return nil, else return sum | ||
| return nil if !all_installed_powers.all? | ||
| return all_installed_powers.sum | ||
| end |
There was a problem hiding this comment.
get_total_installed_power has the same issue as get_total_weighted_average_load: elements[".../LightingSystems/"] returns a single element, but the code treats it like an array (reject, map, all?). Switch to get_elements and parse each LightingSystem's InstalledPower text value.
Comment on lines
+176
to
+190
| # get principal hvac system type | ||
| # @return [String] | ||
| def get_principal_HVAC_system_type | ||
| # if no hvac systems, return nil | ||
| hvac_systems = @base_xml.elements["#{@ns}:Systems/#{@ns}:HVACSystems/"] | ||
| return nil if hvac_systems.nil? | ||
|
|
||
| # find first none nil principal_HVAC_system_type | ||
| hvac_systems = hvac_systems.reject {|s| s.class == REXML::Comment} | ||
| first_principal_HVAC_system_type = hvac_systems.find {|s| s.elements["#{@ns}:PrincipalHVACSystemType/"]} | ||
|
|
||
| # if none, return nil, else return mapping | ||
| return nil if first_principal_HVAC_system_type.nil? | ||
| return BuildingSyncToOSSystemMaps.get_hvac_map[first_principal_HVAC_system_type.to_s] | ||
| end |
There was a problem hiding this comment.
get_principal_HVAC_system_type returns BuildingSyncToOSSystemMaps.get_hvac_map[first_principal_HVAC_system_type.to_s], but first_principal_HVAC_system_type is an XML element; to_s yields serialized XML, not the PrincipalHVACSystemType text. Extract the text value of the PrincipalHVACSystemType element and use that as the mapping key (and handle missing keys explicitly).
Comment on lines
+313
to
+321
| def assert_baseline_osw_exists(baseline_osw_path) | ||
| if !File.file?(baseline_osw_path) | ||
| error_message = ( | ||
| "this function required #{baseline_owm_path}, which does not exist. "\ | ||
| "Create #{baseline_owm_path} with `write_baseline_osw` and try again." | ||
| ) | ||
| OpenStudio.logFree(OpenStudio::Error, "BuildingSync.WorkflowMaker.assert_baseline_osw_exists", error_message) | ||
| raise StandardError, "BuildingSync.WorkflowMaker.assert_baseline_osw_exists: #{error_message}" | ||
| end |
There was a problem hiding this comment.
assert_baseline_osw_exists builds an error message using baseline_owm_path, but the parameter is named baseline_osw_path. This will raise NameError when the file is missing, masking the intended error. Use the correct variable name consistently in the message.
Comment on lines
+91
to
+95
| # read floor_to_floor_height | ||
| floor_to_floor_height_xml = @base_xml.elements["#{@ns}:FloorToFloorHeight"] | ||
| if floor_to_floor_height_xml | ||
| @floor_to_floor_height = floor_to_floor_height_xml.first.to_s.to_f | ||
| end |
There was a problem hiding this comment.
FloorToFloorHeight parsing uses floor_to_floor_height_xml.first.to_s.to_f. first will return the first child node, which may be whitespace/text and is brittle; use floor_to_floor_height_xml.text.to_f (and consider unit handling) to reliably parse the numeric value.
Comment on lines
+1
to
+4
| # insert your copyright here | ||
|
|
||
| # ******************************************************************************* | ||
| # OpenStudio(R), Copyright (c) Alliance for Energy Innovation, LLC. |
There was a problem hiding this comment.
This file still contains the placeholder line # insert your copyright here, which looks like a leftover template artifact. Replace it with the project’s standard header or remove it to avoid shipping placeholder text.
Comment on lines
+386
to
+391
| # Log it | ||
| OpenStudio.logFree( | ||
| OpenStudio::Error, | ||
| 'BuildingSync.WorkflowMaker.write_report_osws', | ||
| "Facility ID: #{@facility.xget_id}. Expected #{@facility.report.poms.length()}, Got #{number_successful} OSWs" | ||
| ) |
There was a problem hiding this comment.
write_report_osws logs with OpenStudio::Error unconditionally, even when the expected/actual counts match. This will make successful runs appear as failures in logs/CI. Only log at error level when the counts differ (or lower the level to Info when successful).
Suggested change
| # Log it | |
| OpenStudio.logFree( | |
| OpenStudio::Error, | |
| 'BuildingSync.WorkflowMaker.write_report_osws', | |
| "Facility ID: #{@facility.xget_id}. Expected #{@facility.report.poms.length()}, Got #{number_successful} OSWs" | |
| ) | |
| expected = @facility.report.poms.length() | |
| # Log it | |
| if number_successful != expected | |
| OpenStudio.logFree( | |
| OpenStudio::Error, | |
| 'BuildingSync.WorkflowMaker.write_report_osws', | |
| "Facility ID: #{@facility.xget_id}. Expected #{expected}, Got #{number_successful} OSWs" | |
| ) | |
| else | |
| OpenStudio.logFree( | |
| OpenStudio::Info, | |
| 'BuildingSync.WorkflowMaker.write_report_osws', | |
| "Facility ID: #{@facility.xget_id}. Successfully wrote #{number_successful} OSWs" | |
| ) | |
| end |
Comment on lines
+16
to
+54
| module BuildingSync | ||
| class BCLWeatherFileDownloader | ||
| @@base_BCL_uri = 'https://bcl.nrel.gov/api/search' | ||
| @@base_EP_uri = 'https://energyplus-weather.s3.amazonaws.com/north_and_central_america_wmo_region_4' | ||
|
|
||
| def self.download_weather_file_from_city_name(city_name, state_name) | ||
| # from BCL, get closest weather station to city | ||
| response = HTTParty.get("#{@@base_BCL_uri}/location:#{city_name.gsub(' ', '+')},#{state_name}.xml?fq=component_tags:\"Weather File\"") | ||
| results = REXML::Document.new(response.body, {ignore_whitespace_nodes: :all, compress_whitespace: :all}).elements["results"] | ||
| results = results.children.filter {|x| x.name == "result"} | ||
|
|
||
| if results.empty? | ||
| raise StandardError, "No weather files found within a 40 mile radius #{city_name},#{state_name} within the BCL." | ||
| end | ||
|
|
||
| # just pick the closest | ||
| result = results[0] | ||
| filenames = result.elements["component/files"].children.map {|x| x.elements["filename"].text} | ||
| filename = filenames[0] | ||
| filename[".epw"] = "" | ||
| state_name = filename.split("_")[1] | ||
|
|
||
| # from ep, get the zip | ||
| uri = "#{@@base_EP_uri}/USA/#{state_name}/#{filename}/#{filename}.zip" | ||
| response = HTTParty.get(uri) | ||
|
|
||
| # extract and write the zip to disk | ||
| FileUtils.mkdir_p(WEATHER_DIR) | ||
| Zip::InputStream.open(::StringIO.new(response.body)) do |zip_stream| | ||
| while entry = zip_stream.get_next_entry | ||
| filepath = File.join(WEATHER_DIR, entry.name) | ||
| File.open(filepath, 'wb') { |file| file.write(entry.get_input_stream.read) } | ||
| end | ||
| end | ||
|
|
||
|
|
||
| return File.join(WEATHER_DIR, filename + ".epw") | ||
| end | ||
| end |
There was a problem hiding this comment.
BCLWeatherFileDownloader is expected to support weather-ID based downloads (the spec references download_weather_file_from_weather_id), but that API isn't implemented here. Add download_weather_file_from_weather_id (and any shared helpers) or remove the unused entry points/tests so the public API is complete and consistent.
Comment on lines
+6
to
+10
| test_configs = [ | ||
| # file_name, standard, epw_path, schema_version | ||
| ['179D_Example_Building.xml', ASHRAE90_1, nil, 'v2.4.0'], # FAILS: weather file???????? | ||
| # ['ASHRAE 211 Export.xml', ASHRAE90_1, nil, 'v2.4.0'], # FAILS: Building has building type Agricultural estate which is not handled by the gem. | ||
| ['BETTER-1.0.0_SampleOffice_gemtest.xml', ASHRAE90_1, nil, 'v2.4.0'], |
There was a problem hiding this comment.
test_configs includes a fixture explicitly marked as failing (179D_Example_Building.xml with comment about weather file). Since the spec iterates over all configs, this will make the test suite fail in CI. Remove this fixture from the active list or conditionally skip it until the underlying issue is fixed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This branch is a significant refactor that strips out individual building-system model articulation classes and replaces them with a simpler, measure-driven workflow approach.
Currently supports OpenStudio version XXX
Deleted Subsystems
The following individual system classes were removed entirely:
hvac_system.rb, lighting_system.rb, loads_system.rb, envelope_system.rb
building_system.rb, service_hot_water_system.rb
wall_system_type.rb, roof_system_type.rb, foundation_system_type.rb, exterior_floor_system_type.rb
helpers/Model.hvac.rb
get_bcl_weather_file.rb (replaced by a new downloader)
New Files
bcl_weather_file_downloader.rb — leaner replacement for weather file retrieval
makers/osw_arg_populator.rb — new class to populate OSW measure arguments
makers/empty_baseline.osw — new empty baseline OSW template (replaces phase_zero_base.osw)
model_articulation/DOE_to_DEER_building_type.rb — DOE-to-DEER building type mapping
model_articulation/systems_map.rb — simplified systems mapping
Heavily Refactored Files
building.rb — massive rework; removed zone hash building, model writing, and building fraction checks; now populates principal_HVAC_system_type and reads climate zone instead of managing detailed system objects
facility.rb — significant rework; streamlined orchestration
workflow_maker.rb — rewritten to create a write_baseline_osw flow rather than the old @workflow-based approach
helpers/helper.rb — substantially slimmed down
workflow_maker_base.rb — simplified base class
Test Changes
Deleted tests for removed subsystems: hvac_system_spec, lighting_system_type_spec, loads_system_spec, envelope_system_spec, service_hot_water_system_spec, translator_sizing_run_spec, translator_spec
Added: translator_write_osw_spec.rb for the new OSW writing flow
Replaced v2.4.0 test fixtures: removed old building_151 files and added many new example files (ASHRAE L1/L2 reports, BETTER, BuildingEQ, NYC, HOMES templates, etc.)
Added small office test case
Build / Packaging
Updated license headers across all files
Renamed gem and updated README
Updated CI workflows