Split emissions layer categories #3917
Open
Conversation
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
Introduced a logger to report errors when required parameters for file requirements are missing in map_layers/base.py. This improves error visibility and debugging for parameter validation.
Refactored the original EmissionsMapLayer into two separate classes: EmbodiedEmissionsMapLayer and OperationalEmissionsMapLayer. Each layer now handles its own data sources, parameters, and file requirements, improving clarity and maintainability. Removed the 'emissions-type' parameter and updated logic to directly reference the appropriate results files for each layer.
WalkthroughRefactors life-cycle emissions map layers into two classes (embodied and operational), adds module logging for map-layer requirements, introduces a new operational emissions timeline schema, renames visualization feature keys and extends plotting labels/units, changes comfort_chart/calc_data signatures to accept building_name and adds multi-building rendering, plus minor comment edits. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as Dashboard UI
participant ML as MapLayer (Embodied / Operational)
participant Loc as Locator
participant FS as Filesystem
UI->>ML: generate_data(params)
ML->>Loc: get zone geometries & centroids
Loc-->>ML: geometries, building list
ML->>Loc: get LCA CSV path (embodied / operational)
Loc-->>ML: CSV path
ML->>FS: read CSV (selected column)
FS-->>ML: rows
ML->>ML: join rows to buildings, compute values & range
ML-->>UI: GeoJSON-like dataset (positions, values, range)
note right of ML: subclass selects label/name and column
sequenceDiagram
autonumber
participant Caller as Caller
participant FR as Base.FileRequirement
participant Log as Logger
Caller->>FR: get_required_files(params)
alt missing params/files
FR->>Log: warn about missing required parameters
FR-->>Caller: raise ValueError
else all present
FR-->>Caller: return required files
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
cea/interfaces/dashboard/map_layers/base.py (1)
15-18: Initialize logger safely to avoid duplicate handlersgetCEAServerLogger unconditionally adds a StreamHandler. If invoked multiple times with the same name (e.g., test reloads), logs may duplicate. Consider guarding in getCEAServerLogger against adding handlers more than once.
Based on relevant code in cea/interfaces/dashboard/lib/logs.py
cea/interfaces/dashboard/map_layers/life_cycle_analysis/layers.py (6)
18-27: Return an empty list instead of None for choicesoptions_generator callers expect an iterable; returning None can cause UI errors. Return [] on missing/empty data.
- try: - emissions_df = pd.read_csv(results_path) - columns = set(emissions_df.columns) - except (pd.errors.EmptyDataError, FileNotFoundError): - return + try: + emissions_df = pd.read_csv(results_path) + columns = set(emissions_df.columns) + except (pd.errors.EmptyDataError, FileNotFoundError): + return []
75-115: Harden data generation: default column, robust range, avoid KeyError, and use self.label
- Use self.label instead of hardcoded strings.
- If 'data-column' missing, pick a sensible default.
- Use reindex to avoid KeyError when CSV/buildings mismatch; filter NaNs in output.
- Compute min/max ignoring NaNs; handle empty safely.
- data_column = parameters['data-column'] + data_column = parameters.get('data-column') + if not data_column: + available = self._get_data_columns() or [] + if not available: + raise ValueError("No available data columns in embodied emissions results.") + data_column = available[0] output = { "data": [], "properties": { "name": self.name, - "label": "Embodied Emissions", + "label": self.label, "description": self.description, "colours": { - "colour_array": [color_to_hex("grey_lighter"), color_to_hex("black")], + "colour_array": [color_to_hex("grey_lighter"), color_to_hex("black")], "points": 12 } } } df = gpd.read_file(self.locator.get_zone_geometry()).set_index("name").loc[buildings] building_centroids = df.geometry.centroid.to_crs(CRS.from_epsg(4326)) results_path = self.locator.get_lca_embodied() - emissions_df = pd.read_csv(results_path, usecols=["name", data_column], index_col="name")[data_column].loc[ - buildings] + emissions_series = ( + pd.read_csv(results_path, usecols=["name", data_column], index_col="name")[data_column] + .reindex(buildings) + ) - output['data'] = [{"position": [centroid.x, centroid.y], "value": emissions} for centroid, emissions in - zip(building_centroids, emissions_df)] + output['data'] = [ + {"position": [centroid.x, centroid.y], "value": float(emissions)} + for centroid, emissions in zip(building_centroids, emissions_series) + if pd.notna(emissions) + ] - output['properties']['range'] = { - 'total': { - 'label': 'Total Range', - 'min': float(min(emissions_df)), - 'max': float(max(emissions_df)) - }, - } + non_na = emissions_series.dropna() + if not non_na.empty: + min_val = float(non_na.min()) + max_val = float(non_na.max()) + else: + min_val = max_val = 0.0 + output['properties']['range'] = { + 'total': { + 'label': 'Total Range', + 'min': min_val, + 'max': max_val + }, + } return output
123-132: Return [] on missing operational results tooMirror the embodied fix: avoid returning None from _get_data_columns.
- try: - emissions_df = pd.read_csv(results_path) - columns = set(emissions_df.columns) - except (pd.errors.EmptyDataError, FileNotFoundError): - return + try: + emissions_df = pd.read_csv(results_path) + columns = set(emissions_df.columns) + except (pd.errors.EmptyDataError, FileNotFoundError): + return []
189-199: Use self.label and keep payload consistentAvoid duplicating literals; derive from class attribute for easier i18n/branding changes.
output = { "data": [], "properties": { "name": self.name, - "label": "Operational Emissions", + "label": self.label, "description": self.description, "colours": { "colour_array": [color_to_hex("grey_lighter"), color_to_hex("black")], "points": 12 } } }
205-218: Match embodied robustness: avoid KeyError, handle NaNs, compute safe rangeAlign operational layer with embodied improvements for consistency and resilience.
- results_path = self.locator.get_lca_operation() - emissions_df = pd.read_csv(results_path, usecols=["name", data_column], index_col="name")[data_column].loc[ - buildings] + results_path = self.locator.get_lca_operation() + emissions_series = ( + pd.read_csv(results_path, usecols=["name", data_column], index_col="name")[data_column] + .reindex(buildings) + ) - output['data'] = [{"position": [centroid.x, centroid.y], "value": emissions} for centroid, emissions in - zip(building_centroids, emissions_df)] - output['properties']['range'] = { - 'total': { - 'label': 'Total Range', - 'min': float(min(emissions_df)), - 'max': float(max(emissions_df)) - }, - } + output['data'] = [ + {"position": [centroid.x, centroid.y], "value": float(emissions)} + for centroid, emissions in zip(building_centroids, emissions_series) + if pd.notna(emissions) + ] + non_na = emissions_series.dropna() + min_val, max_val = (float(non_na.min()), float(non_na.max())) if not non_na.empty else (0.0, 0.0) + output['properties']['range'] = { + 'total': { + 'label': 'Total Range', + 'min': min_val, + 'max': max_val + }, + }
116-122: Reduce duplication between the two emissions layersBoth layers share ~90% of the logic. Consider a small base class (e.g., EmissionsMapLayerBase) with a single abstract method to provide the results path and a shared generate_data implementation. This will reduce maintenance cost and drift.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cea/interfaces/dashboard/map_layers/base.py(2 hunks)cea/interfaces/dashboard/map_layers/life_cycle_analysis/layers.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cea/interfaces/dashboard/map_layers/base.py (1)
cea/interfaces/dashboard/lib/logs.py (1)
getCEAServerLogger(31-39)
cea/interfaces/dashboard/map_layers/life_cycle_analysis/layers.py (3)
cea/interfaces/dashboard/map_layers/base.py (7)
MapLayer(128-219)ParameterDefinition(78-120)file_requirements(161-162)FileRequirement(36-73)cache_output(223-280)generate_data(165-166)expected_parameters(156-157)cea/interfaces/dashboard/map_layers/demand/layers.py (4)
_get_data_columns(55-56)file_requirements(104-114)generate_data(117-183)expected_parameters(63-101)cea/inputlocator.py (4)
get_lca_embodied(1427-1429)get_zone_building_names(902-908)get_zone_geometry(883-887)get_lca_operation(1431-1433)
Comment on lines
48
to
53
| if self.depends_on is not None: | ||
| # Check if the current parameters meet the requirements | ||
| if not all(value in current_params for value in self.depends_on): | ||
| logger.error(f"Missing required parameters for file requirement: {self.depends_on}") | ||
| raise ValueError("Missing required parameters") | ||
|
|
There was a problem hiding this comment.
Guard against None parameters and log more context
Membership test on current_params can raise if None. Also, include locator name and provided params for easier debugging.
Apply:
- if self.depends_on is not None:
- # Check if the current parameters meet the requirements
- if not all(value in current_params for value in self.depends_on):
- logger.error(f"Missing required parameters for file requirement: {self.depends_on}")
- raise ValueError("Missing required parameters")
+ if self.depends_on is not None:
+ # Check if the current parameters meet the requirements
+ missing = [k for k in self.depends_on if not current_params or k not in current_params]
+ if missing:
+ logger.error(
+ f"Missing required parameters for file requirement {self.file_locator}: "
+ f"missing={missing}, provided_keys={list((current_params or {}).keys())}"
+ )
+ raise ValueError(f"Missing required parameters: {missing}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if self.depends_on is not None: | |
| # Check if the current parameters meet the requirements | |
| if not all(value in current_params for value in self.depends_on): | |
| logger.error(f"Missing required parameters for file requirement: {self.depends_on}") | |
| raise ValueError("Missing required parameters") | |
| if self.depends_on is not None: | |
| # Check if the current parameters meet the requirements | |
| missing = [k for k in self.depends_on if not current_params or k not in current_params] | |
| if missing: | |
| logger.error( | |
| f"Missing required parameters for file requirement {self.file_locator}: " | |
| f"missing={missing}, provided_keys={list((current_params or {}).keys())}" | |
| ) | |
| raise ValueError(f"Missing required parameters: {missing}") |
🤖 Prompt for AI Agents
In cea/interfaces/dashboard/map_layers/base.py around lines 48 to 53, guard
against current_params being None before performing membership tests and improve
the error log: first check that current_params is not None (or convert it to an
empty container) before using all(value in current_params for value in
self.depends_on); if the check fails, include the locator name (self.locator or
appropriate attribute) and the actual provided current_params in the
logger.error message and raise a ValueError with a more descriptive message
mentioning the missing parameters and locator context.
Comment on lines
+12
to
+16
| class EmbodiedEmissionsMapLayer(MapLayer): | ||
| category = LifeCycleAnalysisCategory | ||
| name = "life-cycle-analysis" | ||
| label = "GHG Emissions" | ||
| description = "GHG Emissions" | ||
| name = "embodied-emissions" | ||
| label = "Embodied Emissions" | ||
| description = "" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Public rename may break imports—add a back‑compat alias
Renaming EmissionsMapLayer to EmbodiedEmissionsMapLayer is a breaking change. Provide a temporary alias to avoid downstream breakage.
Run this to find existing references:
Add after class definitions:
# Backward compatibility alias (remove in a future major release)
EmissionsMapLayer = EmbodiedEmissionsMapLayer🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -nP -C2 '\bEmissionsMapLayer\b'Length of output: 0
Add backward-compat alias for EmissionsMapLayer
No internal references to EmissionsMapLayer were found, but the public rename is breaking for downstream users. After the EmbodiedEmissionsMapLayer class (e.g. after line 16), add:
# Backward compatibility alias (remove in a future major release)
EmissionsMapLayer = EmbodiedEmissionsMapLayer🤖 Prompt for AI Agents
In cea/interfaces/dashboard/map_layers/life_cycle_analysis/layers.py around
lines 12 to 16, create a backward-compatibility alias for the renamed class by
adding an alias assignment after the EmbodiedEmissionsMapLayer class definition
that binds the old public name EmissionsMapLayer to the new
EmbodiedEmissionsMapLayer, and include a short comment indicating it is for
backward compatibility and slated for removal in a future major release.
Comment on lines
+90
to
+93
| "colours": { | ||
| "colour_array": [color_to_hex("grey_lighter"), color_to_hex("black")], | ||
| "points": 12 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify colours payload shape: 'colour_array' vs 'color_array'
ColourGradient.to_dict() uses 'color_array' (US). Here it's 'colour_array' (UK). Confirm which the frontend expects to avoid broken colour gradients.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Occurrences of colour/color array keys:"
rg -nP -C1 '\b(colo[u]?r_array)\b'
echo "Occurrences of the 'colours' property structure:"
rg -nP -C2 '\bcolou?rs\b'Length of output: 28889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- ColourGradient definition ----"
rg -nP 'class ColourGradient' -C10 cea/interfaces/dashboard/map_layers/base.py
echo "---- ColourGradient.to_dict ----"
rg -nP 'def to_dict' -C5 cea/interfaces/dashboard/map_layers/base.py
echo "---- MapLayer.colours usage in to_dict ----"
rg -nP 'colours' -C5 cea/interfaces/dashboard/map_layers/base.pyLength of output: 1541
Replace ‘colour_array’ with ‘color_array’ in cea/interfaces/dashboard/map_layers/life_cycle_analysis/layers.py:90-93; ColourGradient.to_dict() emits “color_array”, so this must match to avoid broken gradients.
🤖 Prompt for AI Agents
In cea/interfaces/dashboard/map_layers/life_cycle_analysis/layers.py around
lines 90 to 93, the dictionary key "colour_array" must be renamed to
"color_array" to match ColourGradient.to_dict() output; update the key name in
that dict (and any nearby occurrences) so it uses "color_array":
[color_to_hex("grey_lighter"), color_to_hex("black")], leaving values and
structure unchanged.
The hourly loads should already contain only data for the selected building
Updated comfort_chart and calc_data functions to require building_name as an explicit argument instead of inferring it from the data frame. Added a TODO to remove building_name from verify_has_season signature in building_hvac.py.
Refactored the way building-specific HVAC season data is accessed by selecting the row for the building before retrieving column values. This resolves incorrect indexing and ensures proper retrieval of heating and cooling season parameters for the specified building.
Renamed 'emissions-embodied' and 'emissions-operational' to 'embodied-emissions' and 'operational-emissions' in dict_plot_metrics_cea_feature and dict_plot_analytics_cea_feature for consistency.
Raises ValueError with a descriptive message when an invalid plot_cea_feature is provided in title and y_label mapping logic. This improves robustness and helps developers identify missing mappings.
Introduces handling for 'embodied-emissions' in the bar_plot class, including title and y-axis label mappings for various units and normalization options.
Introduces title and y-axis label mappings for the 'operational-emissions' plot feature in the bar_plot class, allowing visualization of operational emissions with various units and normalization options.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cea/visualisation/a_data_loader.py (1)
47-49: Add back-compat aliases for emissions features
- Append the deprecated keys to
dict_plot_analytics_cea_feature:dict_plot_analytics_cea_feature = { …, 'embodied-emissions': [], 'operational-emissions': [] + , 'emissions-embodied': [] # deprecated alias + , 'emissions-operational': [] # deprecated alias }- Update or extend tests to exercise both the new and the alias keys.
process_building_summaryand allInputLocatorpath builders use the raw feature name without special-case mapping, so dashed names are already supported.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cea/demand/building_properties/building_hvac.py(1 hunks)cea/schemas.yml(1 hunks)cea/visualisation/a_data_loader.py(1 hunks)cea/visualisation/c_plotter.py(2 hunks)cea/visualisation/special/comfort_chart.py(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- cea/demand/building_properties/building_hvac.py
🧰 Additional context used
🧬 Code graph analysis (1)
cea/visualisation/special/comfort_chart.py (3)
cea/plots/demand/__init__.py (1)
hourly_loads(117-123)cea/inputlocator.py (1)
get_building_air_conditioning(922-924)cea/demand/building_properties/building_hvac.py (1)
verify_has_season(195-210)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests (ubuntu-latest)
- GitHub Check: tests (windows-latest)
- GitHub Check: tests (macos-latest)
🔇 Additional comments (7)
cea/schemas.yml (1)
19662-19711: LGTMThe schema entry clearly documents the new operational emissions timeline dataset and its column semantics. No issues spotted.
cea/visualisation/special/comfort_chart.py (5)
54-55: LGTM: Data property correctly delegates to cached hourly_loadsReturning self.hourly_loads aligns with DemandSingleBuildingPlotBase.
59-60: LGTM: dict_graph now keyed by self.buildingPassing building into calc_data fixes building-specific lookups.
318-318: LGTM: calc_data is called with building_nameConsistent with the new calc_data signature.
472-485: API update: calc_data signature All internal references updated; no stale two-argument calls found.
304-340: Update callers & docs for new comfort_chart signature
comfort_chart now accepts a sixth argument (locator); ensure all internal/external call sites, examples and user documentation (e.g., notebooks) pass this new parameter.cea/visualisation/a_data_loader.py (1)
38-40: No remaining deprecated emission key references
Verified no ‘emissions-embodied’ or ‘emissions-operational’ usages remain; all producers and consumers now use ‘embodied-emissions’ and ‘operational-emissions’.
Comment on lines
+248
to
+279
| elif plot_cea_feature == 'embodied-emissions': | ||
| if self.y_metric_unit == 'ton' and self.y_normalised_by == 'no_normalisation': | ||
| y_label = "Embodied Emissions (tonnes CO2-eq/yr)" | ||
| elif self.y_metric_unit == 'ton' and self.y_normalised_by == 'gross_floor_area': | ||
| y_label = "Embodied Emissions per Gross Floor Area (tonnes CO2-eq/yr/m2)" | ||
| elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'no_normalisation': | ||
| y_label = "Embodied Emissions (kg CO2-eq/yr)" | ||
| elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'gross_floor_area': | ||
| y_label = "Embodied Emissions per Gross Floor Area (kg CO2-eq/yr/m2)" | ||
| elif self.y_metric_unit == 'g' and self.y_normalised_by == 'no_normalisation': | ||
| y_label = "Embodied Emissions (g CO2-eq/yr)" | ||
| elif self.y_metric_unit == 'g' and self.y_normalised_by == 'gross_floor_area': | ||
| y_label = "Embodied Emissions per Gross Floor Area (g CO2-eq/yr/m2)" | ||
| else: | ||
| raise ValueError(f"Invalid y-metric-unit: {self.y_metric_unit}") | ||
|
|
||
| elif plot_cea_feature == 'operational-emissions': | ||
| if self.y_metric_unit == 'ton' and self.y_normalised_by == 'no_normalisation': | ||
| y_label = "Operational Emissions (tonnes CO2-eq/yr)" | ||
| elif self.y_metric_unit == 'ton' and self.y_normalised_by == 'gross_floor_area': | ||
| y_label = "Operational Emissions per Gross Floor Area (tonnes CO2-eq/yr/m2)" | ||
| elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'no_normalisation': | ||
| y_label = "Operational Emissions (kg CO2-eq/yr)" | ||
| elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'gross_floor_area': | ||
| y_label = "Operational Emissions per Gross Floor Area (kg CO2-eq/yr/m2)" | ||
| elif self.y_metric_unit == 'g' and self.y_normalised_by == 'no_normalisation': | ||
| y_label = "Operational Emissions (g CO2-eq/yr)" | ||
| elif self.y_metric_unit == 'g' and self.y_normalised_by == 'gross_floor_area': | ||
| y_label = "Operational Emissions per Gross Floor Area (g CO2-eq/yr/m2)" | ||
| else: | ||
| raise ValueError(f"Invalid y-metric-unit: {self.y_metric_unit}") | ||
|
|
There was a problem hiding this comment.
Handle conditioned-floor-area normalisation for new emissions plots
The new embodied-emissions and operational-emissions branches don’t cover the existing "conditioned_floor_area" normalisation option (see cea/visualisation/a_data_loader.csv_pointer, Line 122), so choosing that option now raises the fallback ValueError. We need matching labels for that pathway to preserve previous behaviour.
elif plot_cea_feature == 'embodied-emissions':
if self.y_metric_unit == 'ton' and self.y_normalised_by == 'no_normalisation':
y_label = "Embodied Emissions (tonnes CO2-eq/yr)"
+ elif self.y_metric_unit == 'ton' and self.y_normalised_by == 'conditioned_floor_area':
+ y_label = "Embodied Emissions per Conditioned Floor Area (tonnes CO2-eq/yr/m2)"
elif self.y_metric_unit == 'ton' and self.y_normalised_by == 'gross_floor_area':
y_label = "Embodied Emissions per Gross Floor Area (tonnes CO2-eq/yr/m2)"
elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'no_normalisation':
y_label = "Embodied Emissions (kg CO2-eq/yr)"
+ elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'conditioned_floor_area':
+ y_label = "Embodied Emissions per Conditioned Floor Area (kg CO2-eq/yr/m2)"
elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'gross_floor_area':
y_label = "Embodied Emissions per Gross Floor Area (kg CO2-eq/yr/m2)"
elif self.y_metric_unit == 'g' and self.y_normalised_by == 'no_normalisation':
y_label = "Embodied Emissions (g CO2-eq/yr)"
+ elif self.y_metric_unit == 'g' and self.y_normalised_by == 'conditioned_floor_area':
+ y_label = "Embodied Emissions per Conditioned Floor Area (g CO2-eq/yr/m2)"
elif self.y_metric_unit == 'g' and self.y_normalised_by == 'gross_floor_area':
y_label = "Embodied Emissions per Gross Floor Area (g CO2-eq/yr/m2)"
else:
raise ValueError(f"Invalid y-metric-unit: {self.y_metric_unit}")
elif plot_cea_feature == 'operational-emissions':
if self.y_metric_unit == 'ton' and self.y_normalised_by == 'no_normalisation':
y_label = "Operational Emissions (tonnes CO2-eq/yr)"
+ elif self.y_metric_unit == 'ton' and self.y_normalised_by == 'conditioned_floor_area':
+ y_label = "Operational Emissions per Conditioned Floor Area (tonnes CO2-eq/yr/m2)"
elif self.y_metric_unit == 'ton' and self.y_normalised_by == 'gross_floor_area':
y_label = "Operational Emissions per Gross Floor Area (tonnes CO2-eq/yr/m2)"
elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'no_normalisation':
y_label = "Operational Emissions (kg CO2-eq/yr)"
+ elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'conditioned_floor_area':
+ y_label = "Operational Emissions per Conditioned Floor Area (kg CO2-eq/yr/m2)"
elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'gross_floor_area':
y_label = "Operational Emissions per Gross Floor Area (kg CO2-eq/yr/m2)"
elif self.y_metric_unit == 'g' and self.y_normalised_by == 'no_normalisation':
y_label = "Operational Emissions (g CO2-eq/yr)"
+ elif self.y_metric_unit == 'g' and self.y_normalised_by == 'conditioned_floor_area':
+ y_label = "Operational Emissions per Conditioned Floor Area (g CO2-eq/yr/m2)"
elif self.y_metric_unit == 'g' and self.y_normalised_by == 'gross_floor_area':
y_label = "Operational Emissions per Gross Floor Area (g CO2-eq/yr/m2)"
else:
raise ValueError(f"Invalid y-metric-unit: {self.y_metric_unit}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| elif plot_cea_feature == 'embodied-emissions': | |
| if self.y_metric_unit == 'ton' and self.y_normalised_by == 'no_normalisation': | |
| y_label = "Embodied Emissions (tonnes CO2-eq/yr)" | |
| elif self.y_metric_unit == 'ton' and self.y_normalised_by == 'gross_floor_area': | |
| y_label = "Embodied Emissions per Gross Floor Area (tonnes CO2-eq/yr/m2)" | |
| elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'no_normalisation': | |
| y_label = "Embodied Emissions (kg CO2-eq/yr)" | |
| elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'gross_floor_area': | |
| y_label = "Embodied Emissions per Gross Floor Area (kg CO2-eq/yr/m2)" | |
| elif self.y_metric_unit == 'g' and self.y_normalised_by == 'no_normalisation': | |
| y_label = "Embodied Emissions (g CO2-eq/yr)" | |
| elif self.y_metric_unit == 'g' and self.y_normalised_by == 'gross_floor_area': | |
| y_label = "Embodied Emissions per Gross Floor Area (g CO2-eq/yr/m2)" | |
| else: | |
| raise ValueError(f"Invalid y-metric-unit: {self.y_metric_unit}") | |
| elif plot_cea_feature == 'operational-emissions': | |
| if self.y_metric_unit == 'ton' and self.y_normalised_by == 'no_normalisation': | |
| y_label = "Operational Emissions (tonnes CO2-eq/yr)" | |
| elif self.y_metric_unit == 'ton' and self.y_normalised_by == 'gross_floor_area': | |
| y_label = "Operational Emissions per Gross Floor Area (tonnes CO2-eq/yr/m2)" | |
| elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'no_normalisation': | |
| y_label = "Operational Emissions (kg CO2-eq/yr)" | |
| elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'gross_floor_area': | |
| y_label = "Operational Emissions per Gross Floor Area (kg CO2-eq/yr/m2)" | |
| elif self.y_metric_unit == 'g' and self.y_normalised_by == 'no_normalisation': | |
| y_label = "Operational Emissions (g CO2-eq/yr)" | |
| elif self.y_metric_unit == 'g' and self.y_normalised_by == 'gross_floor_area': | |
| y_label = "Operational Emissions per Gross Floor Area (g CO2-eq/yr/m2)" | |
| else: | |
| raise ValueError(f"Invalid y-metric-unit: {self.y_metric_unit}") | |
| elif plot_cea_feature == 'embodied-emissions': | |
| if self.y_metric_unit == 'ton' and self.y_normalised_by == 'no_normalisation': | |
| y_label = "Embodied Emissions (tonnes CO2-eq/yr)" | |
| elif self.y_metric_unit == 'ton' and self.y_normalised_by == 'conditioned_floor_area': | |
| y_label = "Embodied Emissions per Conditioned Floor Area (tonnes CO2-eq/yr/m2)" | |
| elif self.y_metric_unit == 'ton' and self.y_normalised_by == 'gross_floor_area': | |
| y_label = "Embodied Emissions per Gross Floor Area (tonnes CO2-eq/yr/m2)" | |
| elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'no_normalisation': | |
| y_label = "Embodied Emissions (kg CO2-eq/yr)" | |
| elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'conditioned_floor_area': | |
| y_label = "Embodied Emissions per Conditioned Floor Area (kg CO2-eq/yr/m2)" | |
| elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'gross_floor_area': | |
| y_label = "Embodied Emissions per Gross Floor Area (kg CO2-eq/yr/m2)" | |
| elif self.y_metric_unit == 'g' and self.y_normalised_by == 'no_normalisation': | |
| y_label = "Embodied Emissions (g CO2-eq/yr)" | |
| elif self.y_metric_unit == 'g' and self.y_normalised_by == 'conditioned_floor_area': | |
| y_label = "Embodied Emissions per Conditioned Floor Area (g CO2-eq/yr/m2)" | |
| elif self.y_metric_unit == 'g' and self.y_normalised_by == 'gross_floor_area': | |
| y_label = "Embodied Emissions per Gross Floor Area (g CO2-eq/yr/m2)" | |
| else: | |
| raise ValueError(f"Invalid y-metric-unit: {self.y_metric_unit}") | |
| elif plot_cea_feature == 'operational-emissions': | |
| if self.y_metric_unit == 'ton' and self.y_normalised_by == 'no_normalisation': | |
| y_label = "Operational Emissions (tonnes CO2-eq/yr)" | |
| elif self.y_metric_unit == 'ton' and self.y_normalised_by == 'conditioned_floor_area': | |
| y_label = "Operational Emissions per Conditioned Floor Area (tonnes CO2-eq/yr/m2)" | |
| elif self.y_metric_unit == 'ton' and self.y_normalised_by == 'gross_floor_area': | |
| y_label = "Operational Emissions per Gross Floor Area (tonnes CO2-eq/yr/m2)" | |
| elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'no_normalisation': | |
| y_label = "Operational Emissions (kg CO2-eq/yr)" | |
| elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'conditioned_floor_area': | |
| y_label = "Operational Emissions per Conditioned Floor Area (kg CO2-eq/yr/m2)" | |
| elif self.y_metric_unit == 'kg' and self.y_normalised_by == 'gross_floor_area': | |
| y_label = "Operational Emissions per Gross Floor Area (kg CO2-eq/yr/m2)" | |
| elif self.y_metric_unit == 'g' and self.y_normalised_by == 'no_normalisation': | |
| y_label = "Operational Emissions (g CO2-eq/yr)" | |
| elif self.y_metric_unit == 'g' and self.y_normalised_by == 'conditioned_floor_area': | |
| y_label = "Operational Emissions per Conditioned Floor Area (g CO2-eq/yr/m2)" | |
| elif self.y_metric_unit == 'g' and self.y_normalised_by == 'gross_floor_area': | |
| y_label = "Operational Emissions per Gross Floor Area (g CO2-eq/yr/m2)" | |
| else: | |
| raise ValueError(f"Invalid y-metric-unit: {self.y_metric_unit}") |
🤖 Prompt for AI Agents
In cea/visualisation/c_plotter.py around lines 248 to 279 the new branches for
'embodied-emissions' and 'operational-emissions' only handle 'no_normalisation'
and 'gross_floor_area' but not 'conditioned_floor_area', causing the fallback
ValueError when that normalisation is selected; add parallel elif cases for
self.y_normalised_by == 'conditioned_floor_area' for each unit ('ton', 'kg',
'g') that set y_label to the same base text as the corresponding
no_normalisation case but suffixed with "per Conditioned Floor Area (.../m2)"
(e.g. "Embodied Emissions per Conditioned Floor Area (tonnes CO2-eq/yr/m2)" and
similarly for operational emissions and other units) so the
conditioned_floor_area pathway is supported and the ValueError is only raised
for truly invalid units.
Comment on lines
488
to
502
| # read region-specific control parameters (identical for all buildings), i.e. heating and cooling season | ||
| building_name = data_frame.name.iloc[0] | ||
| air_con_data = pd.read_csv(locator.get_building_air_conditioning()).set_index('name') | ||
| air_con_data = pd.read_csv(locator.get_building_air_conditioning()).set_index('name').loc[building_name] | ||
|
|
||
| has_winter = verify_has_season(building_name, | ||
| air_con_data.loc[building_name, 'hvac_heat_starts'], | ||
| air_con_data.loc[building_name, 'hvac_heat_ends']) | ||
| air_con_data.loc['hvac_heat_starts'], | ||
| air_con_data.loc['hvac_heat_ends']) | ||
| has_summer = verify_has_season(building_name, | ||
| air_con_data.loc[building_name, 'hvac_cool_starts'], | ||
| air_con_data.loc[building_name, 'hvac_cool_ends']) | ||
| air_con_data.loc['hvac_cool_starts'], | ||
| air_con_data.loc['hvac_cool_ends']) | ||
|
|
||
| winter_start = air_con_data.loc[building_name, 'hvac_heat_starts'] | ||
| winter_end = air_con_data.loc[building_name, 'hvac_heat_ends'] | ||
| summer_start = air_con_data.loc[building_name, 'hvac_cool_starts'] | ||
| summer_end = air_con_data.loc[building_name, 'hvac_cool_ends'] | ||
| winter_start = air_con_data.loc['hvac_heat_starts'] | ||
| winter_end = air_con_data.loc['hvac_heat_ends'] | ||
| summer_start = air_con_data.loc['hvac_cool_starts'] | ||
| summer_end = air_con_data.loc['hvac_cool_ends'] | ||
|
|
There was a problem hiding this comment.
Harden HVAC lookup for missing buildings and invalid CSVs
Direct .loc[building_name] will raise KeyError if the building is missing; downstream code will crash. Add a safe fallback and clearer error when hvac.csv is absent, plus default “no season” values to proceed gracefully.
Apply:
- air_con_data = pd.read_csv(locator.get_building_air_conditioning()).set_index('name').loc[building_name]
-
- has_winter = verify_has_season(building_name,
- air_con_data.loc['hvac_heat_starts'],
- air_con_data.loc['hvac_heat_ends'])
- has_summer = verify_has_season(building_name,
- air_con_data.loc['hvac_cool_starts'],
- air_con_data.loc['hvac_cool_ends'])
-
- winter_start = air_con_data.loc['hvac_heat_starts']
- winter_end = air_con_data.loc['hvac_heat_ends']
- summer_start = air_con_data.loc['hvac_cool_starts']
- summer_end = air_con_data.loc['hvac_cool_ends']
+ hvac_path = locator.get_building_air_conditioning()
+ try:
+ hvac_df = pd.read_csv(hvac_path).set_index('name')
+ except FileNotFoundError as e:
+ raise FileNotFoundError(f"HVAC configuration not found at {hvac_path}") from e
+
+ if building_name in hvac_df.index:
+ air_con_data = hvac_df.loc[building_name]
+ winter_start = air_con_data['hvac_heat_starts']
+ winter_end = air_con_data['hvac_heat_ends']
+ summer_start = air_con_data['hvac_cool_starts']
+ summer_end = air_con_data['hvac_cool_ends']
+ else:
+ # Fallback: treat as no seasons configured
+ print(f"[comfort_chart] Warning: Building '{building_name}' not found in hvac.csv. Assuming no seasons.")
+ winter_start = winter_end = '00|00'
+ summer_start = summer_end = '00|00'
+
+ has_winter = verify_has_season(building_name, winter_start, winter_end)
+ has_summer = verify_has_season(building_name, summer_start, summer_end)Simplified emissions calculation now always uses the current year for embodied emissions and removes configuration options for year selection and emission timeline. Related parameters have been removed from the default configuration file for clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cea/analysis/lca/main.py (2)
6-6: Adopt module logging and define a loggerUse the standard logger instead of print for consistency and configurability.
Apply this diff near the imports:
-import datetime +import datetime +import logging + +logger = logging.getLogger(__name__)
35-35: Unconditional operational LCA: confirm prerequisites and add loggingThis will fail if demand/supply inputs are missing. If always-on is intentional, fine; otherwise gate or warn when inputs aren’t present. Also log the action.
Apply this diff:
- lca_operation(locator) + logger.info("Running operational emissions (yearly)") + lca_operation(locator)If guarding is desired:
# Example guard (outside diff scope) if os.path.exists(locator.get_total_demand()): lca_operation(locator) else: logger.warning("Skipping operational LCA: demand file not found at %s", locator.get_total_demand())
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cea/analysis/lca/main.py(2 hunks)cea/default.config(0 hunks)
💤 Files with no reviewable changes (1)
- cea/default.config
🧰 Additional context used
🧬 Code graph analysis (1)
cea/analysis/lca/main.py (3)
cea/analysis/lca/embodied.py (1)
lca_embodied(32-217)cea/analysis/lca/operation.py (1)
lca_operation(25-168)cea/analysis/lca/emission_time_dependent.py (2)
operational_hourly(14-41)total_yearly(44-81)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: tests (ubuntu-latest)
- GitHub Check: tests (windows-latest)
- GitHub Check: tests (macos-latest)
🔇 Additional comments (1)
cea/analysis/lca/main.py (1)
29-33: Use a config‐driven year (fallback to now) and switch to logging
Replace the hardcoded datetime.now().year and print with a config lookup, e.g.:- # get current year - year_to_calculate = datetime.datetime.now().year - print(f'Running embodied emissions for year {year_to_calculate}') + # choose year: prefer config.emissions.year_end for reproducibility, else current year + year_to_calculate = getattr(config.emissions, "year_end", None) or datetime.datetime.now().year + logger.info("Running embodied emissions for year %s", year_to_calculate) lca_embodied(year_to_calculate, locator)Optional: if you expect to run for multiple years, parameterize the output path from locator.get_lca_embodied() to avoid overwriting.
Comment on lines
+38
to
+39
| operational_hourly(config) | ||
| total_yearly(config) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Hourly + timeline runs: confirm default building selection and add logging
Running both unconditionally can be heavy; ensure this matches CLI expectations and that config.emissions.buildings has sensible defaults when unset.
Apply this diff:
- operational_hourly(config)
- total_yearly(config)
+ logger.info("Running hourly operational emissions (per-building and district)")
+ operational_hourly(config)
+ logger.info("Running total emissions timeline")
+ total_yearly(config)To verify buildings default semantics:
🏁 Script executed:
#!/bin/bash
# Find where emissions.buildings is defined/used to confirm defaults
rg -n -C3 -P '\bemissions\.buildings\b' --type=pyLength of output: 1230
🏁 Script executed:
#!/bin/bash
# Display imports and logger setup in main.py
sed -n '1,100p' cea/analysis/lca/main.pyLength of output: 1502
🏁 Script executed:
#!/bin/bash
# Show Configuration class and emissions section in cea/config.py
awk '/class Configuration/,/^$/ {print}' cea/config.py
grep -R -n "emissions" -n cea/config.pyLength of output: 110
🏁 Script executed:
#!/bin/bash
# Search cea/config.py for configparser usage and dynamic attribute access
grep -R -n -E "ConfigParser|parser|__getattr__|__getattribute__" cea/config.pyLength of output: 3362
Add logger setup, logging calls and verify default buildings
- In cea/analysis/lca/main.py, import
loggingand add at top:logger = logging.getLogger(__name__)
- Around lines 38–39, wrap calls with logging:
- operational_hourly(config) - total_yearly(config) + logger.info("Running hourly operational emissions (per-building and district)") + operational_hourly(config) + logger.info("Running total emissions timeline") + total_yearly(config)
- Manually confirm that
config.emissions.buildingshas a sensible default in your config file or handle the unset case in code.
🤖 Prompt for AI Agents
In cea/analysis/lca/main.py around lines 38–39, add logging setup at the top by
importing logging and creating logger = logging.getLogger(__name__); before
calling operational_hourly(config) and total_yearly(config) insert
logger.debug/info calls indicating each step start and logger.info on successful
completion (and logger.exception on errors if you wrap calls in try/except);
finally, ensure config.emissions.buildings is validated before use—either check
for presence and set a sensible default or emit a clear logger.warning/error and
return/raise so the code does not proceed with an unset value.
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.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores