Shading setpoint now configurable part of Shading Assembly #3923
Conversation
1. I have implemented the use of two new columns to the shading assembly database. The first (shading_location) allows the user to specify is shading is on the outside or inside of a window. The second allows the user to specify the setpoint of radiation that activates the shading system. Previously this was hardcoded at 300 W/m2. 2. Additionally, I have changed the way total window insolation is calculated. Previously this was treated like walls where the sum of the window radiation for all directions was taken. Now there is a for loop that iterates over each cardinal direction and calculates the insolation for the windows of that direction. This is future proofing for if we decide to treat the shading system and shading factor differently per direction. 3. The calc_blinds_activation function in blinds was taking in radiation as Watts, but needed it as W/m2 (this is the unit for the setpoint). I fixed this so that insolation goes into the function in W/m2 and then later in the parent script (calc_Isol_daysim) is changed back Watts as expected.
WalkthroughReplaces tuple-based DB mappings with a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Solar as Building.SolarCalc
participant EnvDB as Envelope.DB (via DatabaseMapping)
participant Blinds as Technologies.Blinds
note right of EnvDB #F0F8FF: DatabaseMapping supplies file_path, join_column, fields, defaults
Solar->>EnvDB: load shading fields (rf_sh, shading_location, shading_setpoint_wm2)
loop orientations N/E/S/W
Solar->>Solar: compute window area and irradiance (W/m²) per orientation
alt shading_location == "exterior"
Solar->>Blinds: calc_blinds_activation(radiation_wm2,g_gl,Rf_sh,"exterior",setpoint)
Blinds-->>Solar: g_gl adjusted (exterior loss applied pre-entry)
else shading_location == "interior"
Solar->>Blinds: calc_blinds_activation(radiation_wm2,g_gl,Rf_sh,"interior",setpoint)
Blinds-->>Solar: g_gl or g_gl*Rf_sh (based on setpoint)
else unknown
Solar->>Blinds: calc_blinds_activation(..., shading_location=unknown)
Blinds-->>Solar: warning + interior fallback
end
Solar->>Solar: apply frame/g-value/area → accumulate orientation I_sol_win
end
Solar->>Solar: sum walls + roof + windows + underside → I_sol (W)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cea/demand/building_properties/building_envelope.py (1)
85-90: Guard shading DB mapping for backward compatibility (detect available columns).Older scenarios may not have 'shading_location' / 'shading_setpoint_wm2'. If missing, this mapping can fail. Detect available columns and fall back to ['rf_sh'].
Apply this diff to make the mapping robust:
@@ - # Database mappings: (locator_method, join_column_name, columns_to_extract) + # Database mappings: (locator_method, join_column_name, columns_to_extract) + # Detect available shading columns to keep backward compatibility with older schemas + preferred_shading_cols = ['rf_sh', 'shading_location', 'shading_setpoint_wm2'] + try: + shading_db_cols = pd.read_csv(locator.get_database_assemblies_envelope_shading(), nrows=0).columns + shading_cols = [c for c in preferred_shading_cols if c in shading_db_cols] + if not shading_cols: + shading_cols = ['rf_sh'] + except Exception: + shading_cols = ['rf_sh'] @@ 'envelope shading': ( locator.get_database_assemblies_envelope_shading(), 'type_shade', None, - ['rf_sh', 'shading_location', 'shading_setpoint_wm2'] + shading_cols ),Optional: update the docstring bullet for prop_shading to mention
shading_locationandshading_setpoint_wm2.
🧹 Nitpick comments (5)
cea/demand/building_properties/building_solar.py (2)
160-164: Compute underside term once outside the loop.It’s recomputed each iteration and only the last value survives. Move it outside the loop; keep shape using roof series.
Apply this diff:
- #dummy values for base because there's no radiation calculated for bottom-oriented surfaces yet. - I_sol_underside = np.zeros_like(I_sol_win_w_direction) * thermal_resistance_surface['RSE_underside'] + # (no-op here; computed once outside the loop)And add above the loop:
- # initialize total window solar gain - I_sol_win = 0 + # initialize total window solar gain and underside (dummy, no irradiation from underside yet) + I_sol_win = 0 + I_sol_underside = np.zeros_like(I_sol_roof) * thermal_resistance_surface['RSE_underside']
149-154: Avoid np.vectorize if blinds.calc_blinds_activation is array-safe.If
calc_blinds_activationandloss_for_interior_shadingaccept numpy arrays, call directly to avoid Python-loop overhead.Example:
- Fsh_win_direction = np.vectorize(blinds.calc_blinds_activation)(I_sol_win_wm2_direction, - g_gl, - rf_sh, - shading_location=shading_location, - shading_setpoint_wm2=shading_setpoint_wm2) + if shading_location == 'exterior': + Fsh_win_direction = g_gl # scalar + else: + Fsh_win_direction = blinds.calc_blinds_activation( + I_sol_win_wm2_direction, g_gl, rf_sh, + shading_location=shading_location, + shading_setpoint_wm2=shading_setpoint_wm2 + )If not array-safe, keep the current vectorize approach.
cea/schemas.yml (1)
2564-2572: Align setpoint column naming with schema conventions and consider default
- Column name casing is inconsistent with existing
*_Wm2convention (e.g.,Qcsmax_Wm2). Recommendshading_setpoint_Wm2.- If 300 W/m2 is the intended default, encode it explicitly in schema (if supported) or document in description and ensure mapper sets it when missing.
Proposed tweak:
- shading_setpoint_wm2: + shading_setpoint_Wm2: description: Activation setpoint for shading in [W/m2]. When the direct solar radiation on a surface exceeds this value, the shading device - is activated. Defaults to 300 W/m2. + is activated. Default 300 W/m2 if unspecified (set by mapper). type: float unit: '[W/m2]' values: '{1.0...3500.0}' min: 1.0 max: 3500.0If the schema format supports a
defaultfield, prefer adding it instead of relying on description.cea/technologies/blinds.py (2)
23-31: Consider case-insensitive comparison and proper logging.The shading location logic is correct, but there are two minor improvements to consider:
- The string comparison is case-sensitive. Consider normalizing with
.lower()to accept 'Interior', 'INTERIOR', etc.- Using
print()for warnings is not ideal for production code. Consider using Python'sloggingmodule instead.Apply this diff to implement both improvements:
+import logging + +logger = logging.getLogger(__name__) + def calc_blinds_activation(radiation_wm2, g_gl, Rf_sh, shading_location='interior', shading_setpoint_wm2=300): """ This function calculates the blind operation according to ISO 13790. :param radiation_wm2: radiation_wm2 in [W/m2] :param g_gl: window g value :param Rf_sh: shading factor :param shading_location: location of shading device ('interior' or 'exterior' only) (optional, default: 'interior') :param shading_setpoint_wm2: radiation_wm2 setpoint for shading in [W/m2] (optional, default: 300 W/m2) :return: g_gl if no shading, g_gl*Rf_sh if shading is active """ - if shading_location=='interior': + shading_location_normalized = shading_location.lower() + if shading_location_normalized == 'interior': return loss_for_interior_shading(radiation_wm2, g_gl, Rf_sh, shading_setpoint_wm2) - elif shading_location=='exterior': + elif shading_location_normalized == 'exterior': # shading losses calculated before radiation_wm2 enters the window so only loss after that is the g factor # loss is calculated outside of this function return g_gl else: - print(f"Warning: Unrecognized shading location '{shading_location}'. Assuming 'interior'.") + logger.warning(f"Unrecognized shading location '{shading_location}'. Assuming 'interior'.") return loss_for_interior_shading(radiation_wm2, g_gl, Rf_sh, shading_setpoint_wm2)
34-38: LGTM! Helper function is clean and correct.The extracted helper function implements the threshold logic clearly. The logic is straightforward: apply shading factor (Rf_sh) when radiation exceeds the setpoint, otherwise use the base g-value.
Optional enhancement: Consider adding input validation (e.g., checking for negative radiation or ensuring g_gl and Rf_sh are in valid ranges like 0-1) for robustness, though this is not critical.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
cea/databases/CH/ASSEMBLIES/ENVELOPE/ENVELOPE_SHADING.csvis excluded by!**/*.csvcea/databases/DE/ASSEMBLIES/ENVELOPE/ENVELOPE_SHADING.csvis excluded by!**/*.csvcea/databases/SG/ASSEMBLIES/ENVELOPE/ENVELOPE_SHADING.csvis excluded by!**/*.csv
📒 Files selected for processing (4)
cea/demand/building_properties/building_envelope.py(1 hunks)cea/demand/building_properties/building_solar.py(3 hunks)cea/schemas.yml(1 hunks)cea/technologies/blinds.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cea/demand/building_properties/building_solar.py (2)
cea/inputlocator.py (1)
get_radiation_building(1318-1320)cea/technologies/blinds.py (1)
calc_blinds_activation(10-31)
⏰ 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 (macos-latest)
- GitHub Check: tests (windows-latest)
- GitHub Check: tests (ubuntu-latest)
🔇 Additional comments (3)
cea/demand/building_properties/building_solar.py (1)
144-147: Confirm activation threshold semantics (>, or >=?).Should exterior shading trigger at radiation ≥ setpoint instead of strictly >? Align with spec/ISO or project convention.
cea/technologies/blinds.py (2)
10-10: LGTM! Parameter name clarified for units.The function signature now clearly indicates that radiation is in W/m², which fixes the units mismatch described in the PR. The new parameters
shading_locationandshading_setpoint_wm2provide necessary configurability with sensible defaults that preserve legacy behavior.Note: The parameter rename from
radiation_wmtoradiation_wm2is a breaking change for callers using keyword arguments, but this appears intentional per the PR description.
14-21: LGTM! Documentation is clear and accurate.The docstring properly documents the new parameters, clarifies units, and describes the return behavior. The documentation matches the implementation.
| # LEGACY NOTE: shading_location did not exist and shading_setpoint_wm2 was previously hardcoded | ||
| # try-except to manage legacy CSV files without location column in shading assembly description | ||
| try: | ||
| shading_location = prop_envelope.loc[building_name, 'shading_location'].lower() | ||
| except KeyError: | ||
| print("Warning: No shading location found in envelope properties. Assuming 'interior'.") | ||
| shading_location = 'interior' # default value (also default in cea/technologies/blinds.py [calc_blinds_activation]) | ||
|
|
||
| # try-except to manage legacy CSV files without shading_setpoint_wm2 column | ||
| try: | ||
| shading_setpoint_wm2 = prop_envelope.loc[building_name, 'shading_setpoint_wm2'] | ||
| except KeyError: | ||
| print("Warning: No shading setpoint found in envelope properties. Assuming 300 W/m2.") | ||
| shading_setpoint_wm2 = 300 # default value (also default in cea/technologies/blinds.py [calc_blinds_activation]) | ||
|
|
There was a problem hiding this comment.
Handle missing/invalid shading fields and use logging instead of print.
KeyError-only handling misses NaN/invalid values; .lower() on NaN raises. Use robust parsing and defaults; prefer logger warnings.
Apply this diff:
- # try-except to manage legacy CSV files without location column in shading assembly description
- try:
- shading_location = prop_envelope.loc[building_name, 'shading_location'].lower()
- except KeyError:
- print("Warning: No shading location found in envelope properties. Assuming 'interior'.")
- shading_location = 'interior' # default value (also default in cea/technologies/blinds.py [calc_blinds_activation])
+ # shading_location: robust parsing with default
+ raw_location = prop_envelope.loc[building_name].get('shading_location', None)
+ if isinstance(raw_location, str) and raw_location.strip().lower() in ('interior', 'exterior'):
+ shading_location = raw_location.strip().lower()
+ else:
+ logger.warning("No/invalid shading location for %s. Assuming 'interior'.", building_name)
+ shading_location = 'interior'
@@
- # try-except to manage legacy CSV files without shading_setpoint_wm2 column
- try:
- shading_setpoint_wm2 = prop_envelope.loc[building_name, 'shading_setpoint_wm2']
- except KeyError:
- print("Warning: No shading setpoint found in envelope properties. Assuming 300 W/m2.")
- shading_setpoint_wm2 = 300 # default value (also default in cea/technologies/blinds.py [calc_blinds_activation])
+ # shading_setpoint_wm2: robust parsing with default
+ raw_setpoint = prop_envelope.loc[building_name].get('shading_setpoint_wm2', np.nan)
+ try:
+ shading_setpoint_wm2 = float(raw_setpoint)
+ if not np.isfinite(shading_setpoint_wm2) or shading_setpoint_wm2 < 0:
+ raise ValueError
+ except Exception:
+ logger.warning("No/invalid shading setpoint for %s. Assuming 300 W/m2.", building_name)
+ shading_setpoint_wm2 = 300.0Also add a module logger:
+import logging
@@
+logger = logging.getLogger(__name__)Place near the imports at the top of this module.
📝 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.
| # LEGACY NOTE: shading_location did not exist and shading_setpoint_wm2 was previously hardcoded | |
| # try-except to manage legacy CSV files without location column in shading assembly description | |
| try: | |
| shading_location = prop_envelope.loc[building_name, 'shading_location'].lower() | |
| except KeyError: | |
| print("Warning: No shading location found in envelope properties. Assuming 'interior'.") | |
| shading_location = 'interior' # default value (also default in cea/technologies/blinds.py [calc_blinds_activation]) | |
| # try-except to manage legacy CSV files without shading_setpoint_wm2 column | |
| try: | |
| shading_setpoint_wm2 = prop_envelope.loc[building_name, 'shading_setpoint_wm2'] | |
| except KeyError: | |
| print("Warning: No shading setpoint found in envelope properties. Assuming 300 W/m2.") | |
| shading_setpoint_wm2 = 300 # default value (also default in cea/technologies/blinds.py [calc_blinds_activation]) | |
| # At the top of cea/demand/building_properties/building_solar.py, alongside the other imports | |
| import logging | |
| logger = logging.getLogger(__name__) | |
| # … later, around lines 121–135 … | |
| # shading_location: robust parsing with default | |
| raw_location = prop_envelope.loc[building_name].get('shading_location', None) | |
| if isinstance(raw_location, str) and raw_location.strip().lower() in ('interior', 'exterior'): | |
| shading_location = raw_location.strip().lower() | |
| else: | |
| logger.warning("No/invalid shading location for %s. Assuming 'interior'.", building_name) | |
| shading_location = 'interior' | |
| # shading_setpoint_wm2: robust parsing with default | |
| raw_setpoint = prop_envelope.loc[building_name].get('shading_setpoint_wm2', np.nan) | |
| try: | |
| shading_setpoint_wm2 = float(raw_setpoint) | |
| if not np.isfinite(shading_setpoint_wm2) or shading_setpoint_wm2 < 0: | |
| raise ValueError | |
| except Exception: | |
| logger.warning("No/invalid shading setpoint for %s. Assuming 300 W/m2.", building_name) | |
| shading_setpoint_wm2 = 300.0 |
🤖 Prompt for AI Agents
In cea/demand/building_properties/building_solar.py around lines 121 to 135, the
current code only catches KeyError and uses print, which fails for
missing/NaN/invalid values (e.g., .lower() on NaN) and is not using the module
logger; change to use a module-level logger (add logger =
logging.getLogger(__name__) near the imports) and replace the try/except blocks
with robust parsing: check for pandas.isna or use prop_envelope.get/loc with
existence test, coerce the shading_location to string only if non-null before
calling .lower(), default to 'interior' on missing/invalid and log a warning via
logger.warning; for shading_setpoint_wm2, validate numericity (use
pandas.to_numeric or isinstance check), default to 300 when missing/NaN/invalid
and log via logger.warning.
``` the rabbit is nice it helps with pythonic code but write code myself ```
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
cea/demand/building_properties/building_solar.py (1)
123-138: Use module logger instead of print for warnings.The code still uses
print()for warnings, which was flagged in a previous review. Console output is not captured by the application's logging infrastructure.Based on past review comments, apply this diff:
+import logging + +logger = logging.getLogger(__name__)Place near the imports (after line 10), then update the warning statements:
- print(f"No/invalid shading location for {building_name}. Assuming 'interior'.") + logger.warning("No/invalid shading location for %s. Assuming 'interior'.", building_name)- print(f"No/invalid shading setpoint for {building_name}. Assuming 300 W/m2.") + logger.warning("No/invalid shading setpoint for %s. Assuming 300 W/m2.", building_name)
🧹 Nitpick comments (1)
cea/demand/building_properties/building_solar.py (1)
156-156: Add spacing around equality operator for PEP 8 compliance.Apply this diff:
- if shading_location=='exterior': + if shading_location == 'exterior':
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cea/demand/building_properties/building_solar.py(3 hunks)cea/schemas.yml(1 hunks)cea/technologies/blinds.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cea/schemas.yml
🧰 Additional context used
🧬 Code graph analysis (1)
cea/demand/building_properties/building_solar.py (2)
cea/inputlocator.py (1)
get_radiation_building(1318-1320)cea/technologies/blinds.py (1)
calc_blinds_activation(14-35)
⏰ 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). (2)
- GitHub Check: tests (macos-latest)
- GitHub Check: tests (windows-latest)
🔇 Additional comments (1)
cea/technologies/blinds.py (1)
38-53: LGTM!The helper function correctly implements interior shading logic by comparing radiation against the setpoint and returning the appropriate g-value adjustment.
| #dummy values for base because there's no radiation calculated for bottom-oriented surfaces yet. | ||
| I_sol_underside = np.zeros_like(I_sol_win_w_direction) * thermal_resistance_surface['RSE_underside'] |
There was a problem hiding this comment.
Move I_sol_underside initialization outside the direction loop.
I_sol_underside is created inside the per-direction loop, so it gets overwritten on each iteration. The final I_sol sum (line 184) will only include the underside contribution from the last direction processed ('south'), not a proper combined value.
Apply this diff to initialize once before the loop:
# initialize total window solar gain
I_sol_win = 0
+
+ # dummy values for base because there's no radiation calculated for bottom-oriented surfaces yet.
+ # Use a reference shape from one direction (all directions have same time dimension)
+ I_sol_underside = np.zeros(len(radiation_data)) * thermal_resistance_surface['RSE_underside']
for direction in ['east', 'west', 'north', 'south']:Then remove lines 177-178 from inside the loop:
# then reduce by frame factor and g value as usual after radiation has entered the window
# and multiply by window area
I_sol_win_w_direction = (I_sol_win_wm2_direction * Fsh_win_direction * (1 - frame_factor)) * window_area_m2
-
- #dummy values for base because there's no radiation calculated for bottom-oriented surfaces yet.
- I_sol_underside = np.zeros_like(I_sol_win_w_direction) * thermal_resistance_surface['RSE_underside']
# add direction solar heat gain to total window solar gain📝 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.
| #dummy values for base because there's no radiation calculated for bottom-oriented surfaces yet. | |
| I_sol_underside = np.zeros_like(I_sol_win_w_direction) * thermal_resistance_surface['RSE_underside'] | |
| # initialize total window solar gain | |
| I_sol_win = 0 | |
| # dummy values for base because there's no radiation calculated for bottom-oriented surfaces yet. | |
| # Use a reference shape from one direction (all directions have same time dimension) | |
| I_sol_underside = np.zeros(len(radiation_data)) * thermal_resistance_surface['RSE_underside'] | |
| for direction in ['east', 'west', 'north', 'south']: | |
| # ... existing per-direction code ... | |
| I_sol_win_w_direction = ( | |
| I_sol_win_wm2_direction | |
| * Fsh_win_direction | |
| * (1 - frame_factor) | |
| ) * window_area_m2 | |
| # add direction solar heat gain to total window solar gain | |
| I_sol_win += I_sol_win_w_direction |
🤖 Prompt for AI Agents
In cea/demand/building_properties/building_solar.py around lines 177-178, the
I_sol_underside array is initialized inside the per-direction loop which causes
it to be overwritten each iteration; move the initialization of I_sol_underside
(using zeros_like I_sol_win_w_direction multiplied by
thermal_resistance_surface['RSE_underside']) to a single statement immediately
before the direction loop starts, and remove the existing lines 177-178 from
inside the loop so the underside contribution accumulates correctly across all
directions.
cea/technologies/blinds.py
Outdated
| if shading_location=='interior': | ||
| return loss_for_interior_shading(radiation_wm2, g_gl, Rf_sh, shading_setpoint_wm2) | ||
| elif shading_location=='exterior': | ||
| # shading losses calculated before radiation_wm2 enters the window so only loss after that is the g factor | ||
| # loss is calculated outside of this function | ||
| return g_gl | ||
| else: | ||
| print(f"Warning: Unrecognized shading location '{shading_location}'. Assuming 'interior'.") | ||
| return loss_for_interior_shading(radiation_wm2, g_gl, Rf_sh, shading_setpoint_wm2) |
There was a problem hiding this comment.
Replace print with module logger and fix PEP 8 spacing.
Line 34 uses print() for warnings, which should be logged via the module logger. Also, lines 27 and 29 are missing spaces around == operators (PEP 8 compliance).
Apply this diff to add logging:
+import logging
+
+logger = logging.getLogger(__name__)Place near the top of the file, then update the function:
- if shading_location=='interior':
+ if shading_location == 'interior':
return loss_for_interior_shading(radiation_wm2, g_gl, Rf_sh, shading_setpoint_wm2)
- elif shading_location=='exterior':
+ elif shading_location == 'exterior':
# shading losses calculated before radiation_wm2 enters the window so only loss after that is the g factor
# loss is calculated outside of this function
return g_gl
else:
- print(f"Warning: Unrecognized shading location '{shading_location}'. Assuming 'interior'.")
+ logger.warning("Unrecognized shading location '%s'. Assuming 'interior'.", shading_location)
return loss_for_interior_shading(radiation_wm2, g_gl, Rf_sh, shading_setpoint_wm2)📝 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.
| if shading_location=='interior': | |
| return loss_for_interior_shading(radiation_wm2, g_gl, Rf_sh, shading_setpoint_wm2) | |
| elif shading_location=='exterior': | |
| # shading losses calculated before radiation_wm2 enters the window so only loss after that is the g factor | |
| # loss is calculated outside of this function | |
| return g_gl | |
| else: | |
| print(f"Warning: Unrecognized shading location '{shading_location}'. Assuming 'interior'.") | |
| return loss_for_interior_shading(radiation_wm2, g_gl, Rf_sh, shading_setpoint_wm2) | |
| # At the top of cea/technologies/blinds.py | |
| import logging | |
| logger = logging.getLogger(__name__) | |
| if shading_location == 'interior': | |
| return loss_for_interior_shading(radiation_wm2, g_gl, Rf_sh, shading_setpoint_wm2) | |
| elif shading_location == 'exterior': | |
| # shading losses calculated before radiation_wm2 enters the window so only loss after that is the g factor | |
| # loss is calculated outside of this function | |
| return g_gl | |
| else: | |
| logger.warning("Unrecognized shading location '%s'. Assuming 'interior'.", shading_location) | |
| return loss_for_interior_shading(radiation_wm2, g_gl, Rf_sh, shading_setpoint_wm2) |
🤖 Prompt for AI Agents
In cea/technologies/blinds.py around lines 27 to 35, the function uses print()
for warnings and has PEP 8 spacing errors around the '==' operators; replace the
print call with the module logger (e.g., get or create a logger near the top of
the file and use logger.warning(...)) and update the equality checks to include
spaces on both sides of '==' for PEP 8 compliance; add the logger
import/initialization near the top of the file (before the function) and then
change the print(...) line to logger.warning(...) while adjusting spacing in the
if/elif lines.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cea/demand/building_properties/building_solar.py (2)
123-138: Replace print statements with logger warnings.The robust parsing logic is good, but
print()statements should be replaced with proper logging as flagged in previous reviews.Add a module logger at the top of the file:
+import logging + +logger = logging.getLogger(__name__)Then update the warning messages:
- print(f"No or invalid shading location for {building_name}. Assuming 'interior'.") + logger.warning("No or invalid shading location for %s. Assuming 'interior'.", building_name)- print(f"No or invalid shading setpoint for {building_name}. Assuming 300 W/m2.") + logger.warning("No or invalid shading setpoint for %s. Assuming 300 W/m2.", building_name)
179-180: Move I_sol_underside initialization outside the loop.
I_sol_undersideis recreated on every direction iteration, so only the last direction's value survives when summed on Line 186. This causes incorrect underside contribution in the finalI_sol.Initialize once before the loop using a reference shape from the radiation data:
# initialize total window solar gain I_sol_win = 0 + + # dummy values for base because there's no radiation calculated for bottom-oriented surfaces yet. + I_sol_underside = np.zeros(len(radiation_data)) * thermal_resistance_surface['RSE_underside'] for direction in ['east', 'west', 'north', 'south']:Then remove Lines 179-180 from inside the loop:
# then reduce by frame factor and g value as usual after radiation has entered the window # and multiply by window area I_sol_win_w_direction = (I_sol_win_wm2_direction * Fsh_win_direction * (1 - frame_factor)) * window_area_m2 - - #dummy values for base because there's no radiation calculated for bottom-oriented surfaces yet. - I_sol_underside = np.zeros_like(I_sol_win_w_direction) * thermal_resistance_surface['RSE_underside'] # add direction solar heat gain to total window solar gain
🧹 Nitpick comments (3)
cea/demand/building_properties/building_solar.py (3)
148-151: Remove redundant zero assignment and addition.Setting
I_sol_win_w_direction = 0and then adding it toI_sol_winbeforecontinueis unnecessary—adding zero has no effect.Apply this diff:
if window_area_m2 == 0: - I_sol_win_w_direction = 0 - I_sol_win += I_sol_win_w_direction continue # skip to next direction if no window area
157-157: Fix typo in comment."shadying" should be "shading".
Apply this diff:
- # reduce solar radiation by shading and shadying location (interior or exterior) + # reduce solar radiation by shading and shading location (interior or exterior)
158-158: Add space around equality operator.
shading_location=='exterior'should beshading_location == 'exterior'per PEP 8.Apply this diff:
- if shading_location=='exterior': + if shading_location == 'exterior':
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cea/demand/building_properties/base.py(1 hunks)cea/demand/building_properties/building_envelope.py(1 hunks)cea/demand/building_properties/building_solar.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cea/demand/building_properties/building_envelope.py
🧰 Additional context used
🧬 Code graph analysis (1)
cea/demand/building_properties/building_solar.py (2)
cea/inputlocator.py (1)
get_radiation_building(1318-1320)cea/technologies/blinds.py (1)
calc_blinds_activation(14-35)
⏰ 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). (2)
- GitHub Check: tests (windows-latest)
- GitHub Check: tests (macos-latest)
🔇 Additional comments (1)
cea/demand/building_properties/base.py (1)
61-70: LGTM! Clean backward compatibility handling.The legacy support for missing shading fields is well-implemented:
- Correctly scoped to
type_shadejoins only- Provides sensible defaults ('interior' and 300 W/m²) matching the schema
- Extracts fields after applying defaults
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cea/demand/building_properties/building_solar.py (1)
179-180: Critical: Move I_sol_underside initialization outside the direction loop.
I_sol_undersideis reinitialized on every loop iteration, causing it to be overwritten 4 times (once per direction). When line 186 computes the finalI_solsum, it will only include the underside value from the last iteration ('south'), not a proper aggregated or singular underside contribution.Apply this diff to initialize once before the loop:
# initialize total window solar gain I_sol_win = 0 + + # dummy values for base because there's no radiation calculated for bottom-oriented surfaces yet. + I_sol_underside = np.zeros(len(radiation_data)) * thermal_resistance_surface['RSE_underside'] for direction in ['east', 'west', 'north', 'south']:Then remove the initialization from inside the loop:
# then reduce by frame factor and g value as usual after radiation has entered the window # and multiply by window area I_sol_win_w_direction = (I_sol_win_wm2_direction * Fsh_win_direction * (1 - frame_factor)) * window_area_m2 - - #dummy values for base because there's no radiation calculated for bottom-oriented surfaces yet. - I_sol_underside = np.zeros_like(I_sol_win_w_direction) * thermal_resistance_surface['RSE_underside'] # add direction solar heat gain to total window solar gainAs per coding guidelines
🧹 Nitpick comments (3)
cea/demand/building_properties/building_solar.py (3)
121-138: Consider using a logger instead of print statements.While the property extraction logic with defaults is robust, using
print()for warnings in production code limits observability and control. A module-level logger would integrate better with CEA's logging infrastructure.Add a logger at the module level (near the imports):
+import logging + from cea.constants import HOURS_IN_YEAR from cea.demand.sensible_loads import calc_hr, calc_hc from cea.technologies import blinds + +logger = logging.getLogger(__name__)Then update the warning messages:
else: - print(f"No or invalid shading location for {building_name}. Assuming 'interior'.") + logger.warning("No or invalid shading location for %s. Assuming 'interior'.", building_name) shading_location = 'interior' # shading_setpoint_wm2: robust parsing with default raw_setpoint = prop_envelope.loc[building_name].get('shading_setpoint_wm2', np.nan) try: shading_setpoint_wm2 = float(raw_setpoint) if not np.isfinite(shading_setpoint_wm2) or shading_setpoint_wm2 < 0: raise ValueError except Exception: - print(f"No or invalid shading setpoint for {building_name}. Assuming 300 W/m2.") + logger.warning("No or invalid shading setpoint for %s. Assuming 300 W/m2.", building_name) shading_setpoint_wm2 = 300.0Based on learnings
148-151: Remove redundant assignment before continue.Lines 149-150 set
I_sol_win_w_direction = 0and add it toI_sol_winbefore thecontinuestatement. Sincecontinueskips the rest of the loop body andI_sol_win_w_directionis only used later on line 183 (which is never reached), these operations are unnecessary.Apply this diff:
if window_area_m2 == 0: - I_sol_win_w_direction = 0 - I_sol_win += I_sol_win_w_direction continue # skip to next direction if no window area
157-158: Fix typo and add spacing around operator.Minor quality improvements:
- Line 157: typo "shadying" should be "shading"
- Line 158: add spaces around
==operator for consistencyApply this diff:
- # reduce solar radiation by shading and shadying location (interior or exterior) - if shading_location=='exterior': + # reduce solar radiation by shading and shading location (interior or exterior) + if shading_location == 'exterior':
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cea/demand/building_properties/base.py(1 hunks)cea/demand/building_properties/building_envelope.py(1 hunks)cea/demand/building_properties/building_solar.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- cea/demand/building_properties/building_envelope.py
🧰 Additional context used
🧬 Code graph analysis (1)
cea/demand/building_properties/building_solar.py (2)
cea/inputlocator.py (1)
get_radiation_building(1318-1320)cea/technologies/blinds.py (1)
calc_blinds_activation(14-35)
⏰ 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). (1)
- GitHub Check: tests (windows-latest)
🔇 Additional comments (1)
cea/demand/building_properties/base.py (1)
61-67: Approve legacy shading defaults The defaults forshading_location(‘interior’) andshading_setpoint_wm2(300) align with the parameters incalc_blinds_activationand the parsing logic inbuilding_solar.py.
Eliminated the loads_output, massflows_output, and temperatures_output parameters from function calls and definitions in create_unittest_data.py, as they are no longer required. This simplifies the test setup and function signatures.
Introduces a DatabaseMapping dataclass to encapsulate database mapping configuration for building properties. Updates all usages in building_envelope.py, building_hvac.py, and building_supply_systems.py to use the new dataclass, improving code clarity and maintainability.
Introduces an optional 'field_defaults' parameter to DatabaseMapping for handling missing/legacy fields, with logic to add missing columns using these defaults in BuildingPropertiesDatabase. Updates the envelope shading mapping to use this feature and adds documentation outlining a migration plan to move defaults to schemas.yml in the future.
Replaces the use of 'any' with 'Any' from the typing module for the field_defaults attribute in the DatabaseMapping dataclass to ensure correct type hinting.
Introduced a __post_init__ method to DatabaseMapping that raises ValueError if column_renames or field_defaults reference fields not present in the fields list. This ensures data consistency and prevents misconfiguration.
Introduces a check to ensure all required fields exist in the merged DataFrame before slicing. Raises a detailed ValueError if any fields are missing, helping to diagnose issues with database configuration or schema mismatches.
Enhanced the ValueError raised when required fields are missing in the database to include a suggested field_defaults configuration for developers. The new message provides clearer guidance on resolving issues with legacy databases and directs users to relevant documentation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cea/demand/building_properties/MIGRATION_PLAN_SCHEMA_DEFAULTS.md (1)
67-76: Use proper headings instead of bold labelsmarkdownlint (MD036) is warning because the document uses bold text as a heading. Please switch these emphasized labels (e.g., “1.2 Document default policy”) to true
###/####headings so the linter and downstream tooling stay happy.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cea/demand/building_properties/MIGRATION_PLAN_SCHEMA_DEFAULTS.md(1 hunks)cea/demand/building_properties/base.py(3 hunks)cea/demand/building_properties/building_envelope.py(2 hunks)cea/demand/building_properties/building_hvac.py(2 hunks)cea/demand/building_properties/building_supply_systems.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cea/demand/building_properties/building_envelope.py
🧰 Additional context used
🧬 Code graph analysis (2)
cea/demand/building_properties/building_supply_systems.py (2)
cea/demand/building_properties/base.py (2)
BuildingPropertiesDatabase(68-169)DatabaseMapping(13-65)cea/inputlocator.py (4)
get_database_assemblies_supply_heating(793-795)get_database_assemblies_supply_cooling(785-787)get_database_assemblies_supply_hot_water(797-799)get_database_assemblies_supply_electricity(789-791)
cea/demand/building_properties/building_hvac.py (2)
cea/demand/building_properties/base.py (2)
BuildingPropertiesDatabase(68-169)DatabaseMapping(13-65)cea/inputlocator.py (5)
get_database_assemblies_hvac_heating(765-767)get_database_assemblies_hvac_cooling(769-771)get_database_assemblies_hvac_controller(761-763)get_database_assemblies_hvac_hot_water(777-779)get_database_assemblies_hvac_ventilation(773-775)
🪛 markdownlint-cli2 (0.18.1)
cea/demand/building_properties/MIGRATION_PLAN_SCHEMA_DEFAULTS.md
44-44: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
67-67: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
111-111: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
171-171: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
197-197: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
204-204: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
215-215: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
233-233: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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 (windows-latest)
- GitHub Check: tests (ubuntu-latest)
- GitHub Check: tests (macos-latest)
| 'hvac heating': DatabaseMapping( | ||
| file_path=locator.get_database_assemblies_hvac_heating(), | ||
| join_column='hvac_type_hs', | ||
| # TODO: Remove columns from building hvac properties from database filter | ||
| ['class_hs', 'convection_hs', 'Qhsmax_Wm2', 'dThs_C', 'Tshs0_ahu_C', 'dThs0_ahu_C', 'Th_sup_air_ahu_C', | ||
| 'Tshs0_aru_C', 'dThs0_aru_C', 'Th_sup_air_aru_C', 'Tshs0_shu_C', 'dThs0_shu_C'] | ||
| fields=['class_hs', 'convection_hs', 'Qhsmax_Wm2', 'dThs_C', 'Tshs0_ahu_C', 'dThs0_ahu_C', 'Th_sup_air_ahu_C', | ||
| 'Tshs0_aru_C', 'dThs0_aru_C', 'Th_sup_air_aru_C', 'Tshs0_shu_C', 'dThs0_shu_C'] | ||
| ), | ||
| 'hvac cooling': ( | ||
| locator.get_database_assemblies_hvac_cooling(), | ||
| 'hvac_type_cs', | ||
| None, | ||
| ['Qcsmax_Wm2', 'dTcs_C', 'Tscs0_ahu_C', 'dTcs0_ahu_C', 'Tc_sup_air_ahu_C', | ||
| 'Tscs0_aru_C', 'dTcs0_aru_C', 'Tc_sup_air_aru_C', 'Tscs0_scu_C', 'dTcs0_scu_C', | ||
| 'class_cs', 'convection_cs'] | ||
| 'hvac cooling': DatabaseMapping( | ||
| file_path=locator.get_database_assemblies_hvac_cooling(), | ||
| join_column='hvac_type_cs', | ||
| fields=['Qcsmax_Wm2', 'dTcs_C', 'Tscs0_ahu_C', 'dTcs0_ahu_C', 'Tc_sup_air_ahu_C', | ||
| 'Tscs0_aru_C', 'dTcs0_aru_C', 'Tc_sup_air_aru_C', 'Tscs0_scu_C', 'dTcs0_scu_C', | ||
| 'class_cs', 'convection_cs'] | ||
| ), | ||
| 'hvac control': ( | ||
| locator.get_database_assemblies_hvac_controller(), | ||
| 'hvac_type_ctrl', | ||
| None, | ||
| ['dT_Qhs', 'dT_Qcs'] | ||
| 'hvac control': DatabaseMapping( | ||
| file_path=locator.get_database_assemblies_hvac_controller(), | ||
| join_column='hvac_type_ctrl', | ||
| fields=['dT_Qhs', 'dT_Qcs'] | ||
| ), | ||
| 'hvac dhw': ( | ||
| locator.get_database_assemblies_hvac_hot_water(), | ||
| 'hvac_type_dhw', | ||
| None, | ||
| ['class_dhw', 'Tsww0_C', 'Qwwmax_Wm2'] | ||
| 'hvac dhw': DatabaseMapping( | ||
| file_path=locator.get_database_assemblies_hvac_hot_water(), | ||
| join_column='hvac_type_dhw', | ||
| fields=['class_dhw', 'Tsww0_C', 'Qwwmax_Wm2'] | ||
| ), | ||
| 'hvac ventilation': ( | ||
| locator.get_database_assemblies_hvac_ventilation(), | ||
| 'hvac_type_vent', | ||
| None, | ||
| ['MECH_VENT', 'WIN_VENT', 'HEAT_REC', 'NIGHT_FLSH', 'ECONOMIZER'] | ||
| 'hvac ventilation': DatabaseMapping( | ||
| file_path=locator.get_database_assemblies_hvac_ventilation(), | ||
| join_column='hvac_type_vent', | ||
| fields=['MECH_VENT', 'WIN_VENT', 'HEAT_REC', 'NIGHT_FLSH', 'ECONOMIZER'] | ||
| ) |
There was a problem hiding this comment.
Join column names don’t exist in prop_hvac
prop_hvac still exposes the legacy columns (type_hs, type_cs, type_ctrl, type_dhw, type_vent) as shown in the sample table in this module. Switching the mappings to hvac_type_* makes map_database_properties blow up (KeyError when selecting building_properties[[join_column]]). Please keep the join columns on the original names.
Apply this fix:
- 'hvac heating': DatabaseMapping(
- file_path=locator.get_database_assemblies_hvac_heating(),
- join_column='hvac_type_hs',
+ 'hvac heating': DatabaseMapping(
+ file_path=locator.get_database_assemblies_hvac_heating(),
+ join_column='type_hs',
@@
- 'hvac cooling': DatabaseMapping(
- file_path=locator.get_database_assemblies_hvac_cooling(),
- join_column='hvac_type_cs',
+ 'hvac cooling': DatabaseMapping(
+ file_path=locator.get_database_assemblies_hvac_cooling(),
+ join_column='type_cs',
@@
- 'hvac control': DatabaseMapping(
- file_path=locator.get_database_assemblies_hvac_controller(),
- join_column='hvac_type_ctrl',
+ 'hvac control': DatabaseMapping(
+ file_path=locator.get_database_assemblies_hvac_controller(),
+ join_column='type_ctrl',
@@
- 'hvac dhw': DatabaseMapping(
- file_path=locator.get_database_assemblies_hvac_hot_water(),
- join_column='hvac_type_dhw',
+ 'hvac dhw': DatabaseMapping(
+ file_path=locator.get_database_assemblies_hvac_hot_water(),
+ join_column='type_dhw',
@@
- 'hvac ventilation': DatabaseMapping(
- file_path=locator.get_database_assemblies_hvac_ventilation(),
- join_column='hvac_type_vent',
+ 'hvac ventilation': DatabaseMapping(
+ file_path=locator.get_database_assemblies_hvac_ventilation(),
+ join_column='type_vent',📝 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.
| 'hvac heating': DatabaseMapping( | |
| file_path=locator.get_database_assemblies_hvac_heating(), | |
| join_column='hvac_type_hs', | |
| # TODO: Remove columns from building hvac properties from database filter | |
| ['class_hs', 'convection_hs', 'Qhsmax_Wm2', 'dThs_C', 'Tshs0_ahu_C', 'dThs0_ahu_C', 'Th_sup_air_ahu_C', | |
| 'Tshs0_aru_C', 'dThs0_aru_C', 'Th_sup_air_aru_C', 'Tshs0_shu_C', 'dThs0_shu_C'] | |
| fields=['class_hs', 'convection_hs', 'Qhsmax_Wm2', 'dThs_C', 'Tshs0_ahu_C', 'dThs0_ahu_C', 'Th_sup_air_ahu_C', | |
| 'Tshs0_aru_C', 'dThs0_aru_C', 'Th_sup_air_aru_C', 'Tshs0_shu_C', 'dThs0_shu_C'] | |
| ), | |
| 'hvac cooling': ( | |
| locator.get_database_assemblies_hvac_cooling(), | |
| 'hvac_type_cs', | |
| None, | |
| ['Qcsmax_Wm2', 'dTcs_C', 'Tscs0_ahu_C', 'dTcs0_ahu_C', 'Tc_sup_air_ahu_C', | |
| 'Tscs0_aru_C', 'dTcs0_aru_C', 'Tc_sup_air_aru_C', 'Tscs0_scu_C', 'dTcs0_scu_C', | |
| 'class_cs', 'convection_cs'] | |
| 'hvac cooling': DatabaseMapping( | |
| file_path=locator.get_database_assemblies_hvac_cooling(), | |
| join_column='hvac_type_cs', | |
| fields=['Qcsmax_Wm2', 'dTcs_C', 'Tscs0_ahu_C', 'dTcs0_ahu_C', 'Tc_sup_air_ahu_C', | |
| 'Tscs0_aru_C', 'dTcs0_aru_C', 'Tc_sup_air_aru_C', 'Tscs0_scu_C', 'dTcs0_scu_C', | |
| 'class_cs', 'convection_cs'] | |
| ), | |
| 'hvac control': ( | |
| locator.get_database_assemblies_hvac_controller(), | |
| 'hvac_type_ctrl', | |
| None, | |
| ['dT_Qhs', 'dT_Qcs'] | |
| 'hvac control': DatabaseMapping( | |
| file_path=locator.get_database_assemblies_hvac_controller(), | |
| join_column='hvac_type_ctrl', | |
| fields=['dT_Qhs', 'dT_Qcs'] | |
| ), | |
| 'hvac dhw': ( | |
| locator.get_database_assemblies_hvac_hot_water(), | |
| 'hvac_type_dhw', | |
| None, | |
| ['class_dhw', 'Tsww0_C', 'Qwwmax_Wm2'] | |
| 'hvac dhw': DatabaseMapping( | |
| file_path=locator.get_database_assemblies_hvac_hot_water(), | |
| join_column='hvac_type_dhw', | |
| fields=['class_dhw', 'Tsww0_C', 'Qwwmax_Wm2'] | |
| ), | |
| 'hvac ventilation': ( | |
| locator.get_database_assemblies_hvac_ventilation(), | |
| 'hvac_type_vent', | |
| None, | |
| ['MECH_VENT', 'WIN_VENT', 'HEAT_REC', 'NIGHT_FLSH', 'ECONOMIZER'] | |
| 'hvac ventilation': DatabaseMapping( | |
| file_path=locator.get_database_assemblies_hvac_ventilation(), | |
| join_column='hvac_type_vent', | |
| fields=['MECH_VENT', 'WIN_VENT', 'HEAT_REC', 'NIGHT_FLSH', 'ECONOMIZER'] | |
| ) | |
| 'hvac heating': DatabaseMapping( | |
| file_path=locator.get_database_assemblies_hvac_heating(), | |
| join_column='type_hs', | |
| # TODO: Remove columns from building hvac properties from database filter | |
| fields=['class_hs', 'convection_hs', 'Qhsmax_Wm2', 'dThs_C', 'Tshs0_ahu_C', 'dThs0_ahu_C', 'Th_sup_air_ahu_C', | |
| 'Tshs0_aru_C', 'dThs0_aru_C', 'Th_sup_air_aru_C', 'Tshs0_shu_C', 'dThs0_shu_C'] | |
| ), | |
| 'hvac cooling': DatabaseMapping( | |
| file_path=locator.get_database_assemblies_hvac_cooling(), | |
| join_column='type_cs', | |
| fields=['Qcsmax_Wm2', 'dTcs_C', 'Tscs0_ahu_C', 'dTcs0_ahu_C', 'Tc_sup_air_ahu_C', | |
| 'Tscs0_aru_C', 'dTcs0_aru_C', 'Tc_sup_air_aru_C', 'Tscs0_scu_C', 'dTcs0_scu_C', | |
| 'class_cs', 'convection_cs'] | |
| ), | |
| 'hvac control': DatabaseMapping( | |
| file_path=locator.get_database_assemblies_hvac_controller(), | |
| join_column='type_ctrl', | |
| fields=['dT_Qhs', 'dT_Qcs'] | |
| ), | |
| 'hvac dhw': DatabaseMapping( | |
| file_path=locator.get_database_assemblies_hvac_hot_water(), | |
| join_column='type_dhw', | |
| fields=['class_dhw', 'Tsww0_C', 'Qwwmax_Wm2'] | |
| ), | |
| 'hvac ventilation': DatabaseMapping( | |
| file_path=locator.get_database_assemblies_hvac_ventilation(), | |
| join_column='type_vent', | |
| fields=['MECH_VENT', 'WIN_VENT', 'HEAT_REC', 'NIGHT_FLSH', 'ECONOMIZER'] | |
| ) |
Updated the equality checks for shading_location to include spaces around the '==' operator for improved code readability and consistency.
Updated assertion error messages in test_calc_thermal_loads.py to use f-strings for improved readability and clarity.
|
I just realised the test assertion statements were very misleading (when comparing expected values).
Summary:
@justinfmccarty just to confirm, it makes sense for heating demand to go down? |
|
Hey. On the bus. So generally speaking the blinds are now going to activate less often. This means that around the year there is more sunlight entering the building. This leads to less heating requirement in the cold months and more cooling requirement in the warm months. This has to do with the change in the unit of the input value to blinds.calc_activation from kWh to W/m2. But I will confirm that change is definitely right tonight. And now that the input is accidentally coded as kWh/m2. But when the blinds are activated the math doesn't change for the output radiation |
|
Hmm, this is not good news as CEA tend to under estimate heating demand rather than over estimate. |
|
@reyery and @ShiZhongming, I don't really know what else to look at. The changes aren't misbehaving. You can see in the screenshot of the related scripts in 'master' that the calc_blinds_activation was intaking radiation in Watts when it was making a test against a factor specified in Wm2. While I changed other things, fixing the units is what we are seeing impact the results. Prior to the change then the input to the calc would have been like 26,000 Watts instead of something like 600 W/m2 which it needs to be. Perhaps we need to make an effort to understand why the heating demand is so low in general. But I have taken a look again and find that my units make sense from the radiation_data csv read to the blinds calc and then to total solar gains. Anything else you can think to check? 
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cea/demand/building_properties/building_solar.py (2)
121-138: Reminder: Use module logger instead of print statements.This was flagged in a previous review but remains unaddressed. The code still uses
print()for warnings (lines 127, 137) instead of a module-level logger. In a production system, structured logging is preferred over print statements.As previously suggested, apply this diff to use logging:
+import logging + +logger = logging.getLogger(__name__)Then replace the print statements:
- print(f"No or invalid shading location for {building_name}. Assuming 'interior'.") + logger.warning("No/invalid shading location for %s. Assuming 'interior'.", building_name)- print(f"No or invalid shading setpoint for {building_name}. Assuming 300 W/m2.") + logger.warning("No/invalid shading setpoint for %s. Assuming 300 W/m2.", building_name)
182-183: Reminder: Move I_sol_underside initialization outside the loop.This was flagged in a previous review but remains unaddressed.
I_sol_undersideis initialized inside the per-direction loop and gets overwritten on each iteration. Only the last direction's value is used in the final sum (line 189). While this doesn't affect correctness (since it's zeros), it's inefficient and confusing.As previously suggested, initialize once before the loop:
# initialize total window solar gain I_sol_win = 0 + + # dummy values for base because there's no radiation calculated for bottom-oriented surfaces yet. + I_sol_underside = np.zeros(len(radiation_data)) * thermal_resistance_surface['RSE_underside'] for direction in ['east', 'west', 'north', 'south']:Then remove lines 182-183 from inside the loop.
🧹 Nitpick comments (5)
cea/demand/building_properties/building_solar.py (2)
151-154: Remove redundant zero assignment and addition.Lines 152-153 assign 0 to
I_sol_win_w_directionand add it toI_sol_winbefore continuing. Since adding 0 has no effect and the variable is not used outside this block, these lines are unnecessary.Apply this diff to simplify:
if actual_window_area_m2 == 0: - I_sol_win_w_direction = 0 - I_sol_win += I_sol_win_w_direction continue # skip to next direction if no window area
161-161: Add space around equality operator.PEP 8 recommends spaces around comparison operators.
Apply this diff:
- if shading_location=='exterior': + if shading_location == 'exterior':cea/schemas.yml (3)
2548-2556: Use unit 'NA' for strings and drop redundant values
shading_locationshould follow schema conventions for string enums:
- unit: 'NA' (not '[-]')
- avoid redundant
values: alphanumericwhenchoice.valuesis presentProposed patch:
- shading_location: - description: Location of shading device ('interior' or 'exterior' only) - type: string - unit: '[-]' - choice: - values: - - interior - - exterior - values: alphanumeric + shading_location: + description: Location of shading device ('interior' or 'exterior' only) + type: string + unit: 'NA' + choice: + values: + - interior + - exterior
2558-2563: Clarify rf_sh description grammar (optional)Consider minor grammar tweaks for readability:
- “If exterior blinds, incident window radiation is multiplied by this factor. If interior blinds, the window G-value is multiplied by this factor and the incident radiation is multiplied by the resulting product.”
2569-2577: Confirm naming and default handling for shading_setpoint_wm2
- Naming: many fields use
_Wm2(capital W). If consistency is desired, considershading_setpoint_Wm2. Only change if all call sites are updated or an alias is provided.- Default: description states default 300 W/m2. If the loader honors schema defaults, consider adding an explicit default; else ensure code sets it.
Do you want to align the name casing or keep as-is for compatibility?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cea/demand/building_properties/building_solar.py(3 hunks)cea/schemas.yml(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cea/demand/building_properties/building_solar.py (2)
cea/inputlocator.py (1)
get_radiation_building(1318-1320)cea/technologies/blinds.py (1)
calc_blinds_activation(14-35)
⏰ 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). (2)
- GitHub Check: tests (windows-latest)
- GitHub Check: tests (macos-latest)
🔇 Additional comments (1)
cea/schemas.yml (1)
2719-2720: G_win description clarification LGTMClearer meaning of “1” is helpful. No further action.
|
I have changed the base branch to |
4 participants
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests