replace py4design in radiation_daysim with compas #3919
Conversation
Also rename it to BuildingGeometryForRadiation, because another already exists for thermal loads.
WalkthroughAdds a COMPAS-typed radiation geometry model (BuildingGeometryForRadiation and SurfaceGroup), a polygon patch partitioner, and migrates Daysim, Radiance, geometry-generation, CRAX, and main flows to use the new typed APIs, updated signatures, and pickle-based serialization. Dependencies updated to include compas and compas-libigl. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Main as radiation.main
participant Geo as geometry_generator
participant BG as BuildingGeometryForRadiation
participant Daysim as radiation.daysim
participant SAC as sensor_area_calculator
participant Rad as radiance.create_rad_geometry
rect rgba(220,235,255,0.6)
User->>Main: run_daysim_simulation(building_names,...)
Main->>Geo: geometry_main(..., geometry_pickle_dir)
Geo-->>Main: terrain_mesh, zone_names, surroundings, tree_polys
end
loop per building
Main->>BG: BG.load(pickle_path)
BG-->>Daysim: group(srf_type) => faces, orientations, normals, intersects
Daysim->>SAC: partition_polygon_by_grid(face, grid_dx, grid_dy)
SAC-->>Daysim: patches
Daysim-->>Main: assembled sensor coords, dirs, types, areas, labels, intersections
end
Main->>Rad: create_rad_geometry(file, terrain_mesh, df, zone_names, surroundings, pickle_dir)
Rad-->>User: radiance scene files
sequenceDiagram
autonumber
participant BG as BuildingGeometryForRadiation
participant Disk as Filesystem
rect rgb(240,250,240)
BG->>BG: __getstate__() %% serialize ordered fields
BG->>Disk: save(pickle_path) %% ensure dir and write pickle
Disk-->>BG: pickle saved
end
Disk->>BG: load(pickle_path)
BG->>BG: __setstate__(loaded_state) %% validate & restore fields
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 (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 (2)
cea/resources/radiation/radiance.py (1)
103-118: Clamp leaf area density and guard sqrt domainsqrt(1 - lad) will error for lad>1 or lad<0; also negative transmissivity is invalid.
Apply:
- transmissivity = math.sqrt(1 - leaf_area_densities[i]) + lad = max(0.0, min(1.0, float(leaf_area_densities[i]))) + transmissivity = math.sqrt(max(0.0, 1.0 - lad))cea/resources/radiation/geometry_generator.py (1)
178-225: Guard against zero or negative floors and inconsistent attributesnfloors=0 will cause division by zero; void_deck>nfloors already checked but nonpositive floors should error early.
Apply:
height = buildings_df['height_ag'].astype(float) - nfloors = buildings_df['floors_ag'].astype(int) + nfloors = buildings_df['floors_ag'].astype(int) void_decks = buildings_df['void_deck'].astype(int) + if not all(nfloors > 0): + bad = buildings_df.index[nfloors <= 0].tolist() + raise ValueError(f"floors_ag must be > 0 for all buildings. Invalid: {bad}")
🧹 Nitpick comments (6)
cea/resources/radiation/radiance.py (2)
486-487: Docstring type is inconsistent with new COMPAS typesParam doc still references OCC; should be Polygon.
Apply:
- :param OCC.TopoDS.TopoDS_Face face: Polygon (OCC TopoDS_Face) of surface + :param Polygon face: Polygon of surface
134-141: Windows path join + stderr decodingUse os.path.join for the executable on Windows; decode stderr before printing.
Apply:
- _cmd = shlex.split(cmd) - if sys.platform == "win32": - # Prepend daysim directory to binary for windows since PATH cannot be changed using env - # Refer to https://docs.python.org/3/library/subprocess.html#popen-constructor - _cmd[0] = f"{daysim_dir}\\{_cmd[0]}" + _cmd = shlex.split(cmd) + if sys.platform == "win32": + _cmd[0] = os.path.join(daysim_dir, _cmd[0]) @@ - if process.returncode != 0: - print(process.stderr) + if process.returncode != 0: + print(process.stderr.decode("utf-8", errors="replace")) raise subprocess.CalledProcessError(process.returncode, cmd)Also applies to: 146-147
cea/resources/radiation/geometry_generator.py (4)
604-607: Exact Z-equality check is fragile; use toleranceFloating point Z equality can fail even for coplanar floors.
Apply:
- if not all(p.z == floor.points[0].z for p in floor.points): - raise ValueError("The floor polygon must be on the XY plane, all points must have the same Z coordinate.") + z0 = floor.points[0].z + if not all(abs(p.z - z0) <= 1e-6 for p in floor.points): + raise ValueError("The floor polygon must be planar and parallel to XY (constant Z).")
150-177: Scale around centroid; avoid extra translate and reduce driftUse origin parameter to scale in place; keeps window perfectly centered.
Apply:
- window: Polygon = surface.scaled(math.sqrt(wwr)) - window.translate(Vector.from_start_end(window.centroid, surface.centroid)) + window: Polygon = surface.scaled(math.sqrt(wwr), origin=surface.centroid)Note: consider offset-based insetting for concave faces if needed in future.
118-132: Docstring and type hints mismatch with behaviorFunction raises on no hit but docstring says returns (None, None). Align docs or behavior.
Apply (update docstring):
- :return: a tuple containing the index of the face that was hit and the intersection point. - if no intersection was found, it returns (None, None). - :rtype: tuple[int | None, Point | None] + :return: (face_index, intersection_point). Raises ValueError if no intersection is found. + :rtype: tuple[int, Point]Also applies to: 148-148
376-385: Nit: naming and simplificationRename for readability and avoid redundant branches.
Apply:
-def are_buildings_close_to_eachother(x_1, y_1, solid2: OCCBrep, dist=100): +def are_buildings_close_to_each_other(x_1, y_1, solid2: OCCBrep, dist=100.0) -> bool: @@ - if delta <= dist: - return True - else: - return False + return delta <= distRemember to update the single call site accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cea/resources/radiation/building_geometry_radiation.py(1 hunks)cea/resources/radiation/daysim.py(6 hunks)cea/resources/radiation/geometry_generator.py(24 hunks)cea/resources/radiation/main.py(2 hunks)cea/resources/radiation/radiance.py(7 hunks)cea/resources/radiation/sensor_area_calculator.py(1 hunks)cea/resources/radiationCRAX/main.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
cea/resources/radiation/main.py (3)
cea/resources/radiation/radiance.py (1)
CEADaySim(31-205)cea/inputlocator.py (1)
get_weather_file(651-654)cea/utilities/epwreader.py (1)
epw_reader(41-73)
cea/resources/radiation/daysim.py (3)
cea/resources/radiation/building_geometry_radiation.py (3)
BuildingGeometryForRadiation(20-119)group(50-77)load(104-113)cea/resources/radiation/sensor_area_calculator.py (2)
build_sensor_patches(7-122)patch_centers_from_patches(125-149)cea/inputlocator.py (1)
get_radiation_metadata(1331-1333)
cea/resources/radiation/geometry_generator.py (4)
cea/resources/radiation/building_geometry_radiation.py (3)
BuildingGeometryForRadiation(20-119)from_dict(87-101)save(115-119)cea/utilities/standardize_coordinates.py (3)
crs_to_epsg(68-72)get_lat_lon_projected_shapefile(75-90)get_projected_coordinate_system(55-65)cea/config.py (1)
get_number_of_processes(246-257)cea/inputlocator.py (5)
get_zone_geometry(883-887)get_surroundings_geometry(895-900)get_tree_geometry(878-881)get_terrain(980-982)get_building_architecture(926-931)
cea/resources/radiation/radiance.py (1)
cea/resources/radiation/building_geometry_radiation.py (2)
BuildingGeometryForRadiation(20-119)load(104-113)
cea/resources/radiationCRAX/main.py (5)
cea/datamanagement/utils/__init__.py (1)
migrate_void_deck_data(12-49)cea/resources/radiation/daysim.py (2)
GridSize(55-57)calc_sensors_building(182-235)cea/resources/radiation/building_geometry_radiation.py (2)
BuildingGeometryForRadiation(20-119)load(104-113)cea/inputlocator.py (5)
ensure_parent_folder_exists(104-106)get_radiation_metadata(1331-1333)get_solar_radiation_folder(1319-1321)get_building_architecture(926-931)InputLocator(20-1505)cea/resources/radiationCRAX/CRAXModel.py (4)
check_crax_exe_directory(142-221)CRAX(25-139)run_mesh_generation(77-98)run_radiation(100-139)
⏰ 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/resources/radiation/geometry_generator.py (1)
785-804: Mesh.from_points already performs a Delaunay triangulation.COMPAS’s Mesh.from_points constructs a mesh via a Delaunay triangulation of the input points in the XY plane, not just a convex hull, so the existing implementation already produces a valid terrain TIN.
Likely an incorrect or invalid review comment.
|
When the sensor density is very high (1m for both roof and wall), this is what it looks like (each building takes ~80s on my computer with single processor): |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
pyproject.toml(2 hunks)
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
cea/resources/radiation/geometry_generator.py (5)
462-473: Clarify the need for complex number handling.The
safe_floathelper checks for complex values when extracting WWR (window-to-wall ratio) data. WWR should always be real-valued (0 to 1). Is this check necessary, or does it indicate a data quality issue upstream?Consider simplifying if complex values are not expected:
def safe_float(val): - # Convert numpy scalar or complex to float, raise error if complex part is nonzero - if hasattr(val, 'real'): - if getattr(val, 'imag', 0) != 0: - raise ValueError(f"Cannot convert complex value {val} to float.") - return float(val.real) - return float(val) + # Convert numpy scalar to float + if hasattr(val, 'item'): + return float(val.item()) + return float(val)
595-596: Refine error message precision.The error message states "must be on the XY plane" but the validation only checks that all Z coordinates are equal. The floor can be at any elevation (e.g., Z=10), not necessarily Z=0. Consider clarifying the message.
Apply this diff:
- raise ValueError("The floor polygon must be on the XY plane, all points must have the same Z coordinate.") + raise ValueError("The floor polygon must be planar (parallel to XY plane); all points must have the same Z coordinate.")
658-658: Document the offset distance magic number.The offset value
0.1(meters) is used to position the test point slightly outside the surface for intersection testing. Consider making this a named constant or documenting why this specific value was chosen.Apply this diff to add a module-level constant:
+# Offset distance (m) for positioning test points outside surfaces during intersection checks +INTERSECTION_TEST_OFFSET = 0.1 + def calc_windows_walls(facade_list: list[Polygon], wwr: float, potentially_intersecting_solids_faces: list[list[Polygon]],Then use it at line 658:
- data_point: Point = ref_pypt.translated(standard_normal.scaled(0.1)) + data_point: Point = ref_pypt.translated(standard_normal.scaled(INTERSECTION_TEST_OFFSET))
682-697: Simplify redundant ternary operations.The ternary expressions at lines 682-687 and 697 are redundant since
intersectsis already guaranteed to be 0 or 1 (fromcalc_intersection_face_solid). These checks don't add value and can be simplified.Apply these diffs:
- wall_intersects.extend( - [ - intersects if intersects in (0, 1) else 0 - for _ in range(len(hollowed_facades)) - ] - ) + wall_intersects.extend([intersects] * len(hollowed_facades))- wall_intersects.append(1 if intersects else 0) + wall_intersects.append(intersects)
846-858: Consider consistent multiprocessing approach.This function uses
multiprocessing.Pooldirectly, while building geometry generation usescea.utilities.parallel.vectorize(line 214). Consider using the same approach for consistency, unless there's a specific reason for the difference.If you want to unify the approach, you could refactor to use
cea.utilities.parallel.vectorize:- from multiprocessing import cpu_count - from multiprocessing.pool import Pool + from itertools import repeat - with Pool(cpu_count() - 1) as pool: - surfaces = [ - solid_faces - for (solid_faces, _) in pool.starmap( - process_geometries, - ( - (geom, elevation_map, range(1), z) - for geom, z in zip(tree_df["geometry"], tree_df["height_tc"]) - ), - ) - ] + n = len(tree_df) + out = cea.utilities.parallel.vectorize(process_geometries, config.get_number_of_processes())( + tree_df["geometry"], + repeat(elevation_map, n), + repeat(range(1), n), + tree_df["height_tc"] + ) + surfaces = [solid_faces for solid_faces, _ in out]However, if
configis not available in this function, the current approach is acceptable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cea/resources/radiation/geometry_generator.py(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cea/resources/radiation/geometry_generator.py (2)
cea/resources/radiation/building_geometry_radiation.py (3)
BuildingGeometryForRadiation(35-137)from_dict(103-117)save(131-137)cea/utilities/standardize_coordinates.py (3)
crs_to_epsg(68-72)get_lat_lon_projected_shapefile(75-90)get_projected_coordinate_system(55-65)
🔇 Additional comments (16)
cea/resources/radiation/geometry_generator.py (16)
1-59: LGTM!The import structure correctly reflects the migration from py4design/OCC to COMPAS geometry framework. Type hints and new utility imports are appropriately included.
61-114: LGTM!The surface classification logic correctly migrates to COMPAS Polygon types while preserving the angle-based classification algorithm. The use of
angle_signedwith right-hand rule normal is appropriate for computing facade orientations.
117-146: LGTM!The ray-mesh intersection logic correctly implements barycentric coordinate interpolation to find the Z elevation at the intersection point. Using the first hit (
hits[0]) is appropriate for finding the closest terrain intersection.
148-171: LGTM!The window creation logic correctly scales the surface by
sqrt(wwr)to maintain the area ratio, then constructs trapezoid wall segments around the window. The vertex ordering maintains proper polygon winding.
173-220: LGTM!The function correctly generates exterior polygon faces for each building using the refactored COMPAS-based approach. The handling of void_deck floors and multiprocessing integration are appropriate.
222-252: LGTM!The function correctly converts a 2D footprint into exterior polygons by sampling terrain elevation and extruding floors. Returning plain polygons instead of OCC breps maintains pickle compatibility for multiprocessing.
255-278: LGTM!The function correctly constructs
BuildingGeometryForRadiationinstances for surrounding buildings using the new polygon-based approach. Surface classification and serialization are appropriate.
281-365: LGTM!The function correctly orchestrates the 2D-to-3D conversion for zone and surrounding buildings using the new COMPAS-based polygon approach. Empty surroundings handling is appropriate.
376-394: LGTM!The helper functions correctly compute bounding boxes and proximity checks using COMPAS Point types. The logic is straightforward and correct.
513-544: LGTM!The function correctly samples terrain elevation at the footprint centroid and translates the footprint polygon to that elevation. The use of ray-mesh intersection is appropriate.
547-576: LGTM!The function correctly extrudes a footprint into stacked storeys using temporary OCC breps for boolean union, then returns the exterior faces as polygons. This approach maintains pickle compatibility while leveraging robust boolean operations.
702-722: LGTM!The function correctly uses OCCBrep and direct OCC calls for robust point-in-solid classification. Returning a boolean-like 0/1 value is appropriate for the intersection flag.
794-798: Verify terrain triangulation behavior.The code uses
Mesh.from_pointsto generate a TIN from raster elevation data. Ensure this produces appropriate Delaunay triangulation for terrain representation and doesn't inadvertently create a convex hull or other unintended structure.Based on learnings,
compas.datastructures.Mesh.from_pointsshould create a proper mesh from points, but confirming the triangulation algorithm used would be helpful. You may want to validate the terrain mesh output matches expectations for your specific use cases.
854-854: LGTM! Past review comment addressed.Using
range(1)correctly generates a single-floor solid for trees (slab at Z=0, ceiling at Z=height), resolving the previous concern about height doubling. The tree geometry now has the correct height.
863-926: LGTM!The main geometry function correctly orchestrates the complete geometry generation pipeline using COMPAS-based types. Return values properly reflect the new polygon-based architecture.
929-1003: LGTM!The main block provides a basic usage example and includes commented-out visualization code for debugging purposes. The active code correctly demonstrates the geometry generation workflow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
cea/resources/radiation/geometry_generator.py (6)
847-859: Prefercea.utilities.parallel.vectorizefor consistency with CPU configuration.Direct use of
multiprocessing.pool.Poolwithcpu_count() - 1bypasses the centralized CPU management inconfig.get_number_of_processes(), which respectsconfig.number_of_CPUs_to_keep_free. This creates inconsistency with building geometry generation (line 214) and may not honor user preferences.Consider refactoring to:
- from multiprocessing import cpu_count - from multiprocessing.pool import Pool - - with Pool(cpu_count() - 1) as pool: - surfaces = [ - solid_faces - for (solid_faces, _) in pool.starmap( - process_geometries, - ( - (geom, elevation_map, range(1), z) - for geom, z in zip(tree_df["geometry"], tree_df["height_tc"]) - ), - ) - ] + # Assume config is passed as a parameter or accessible + num_processes = 1 # or pass config to get_number_of_processes() + n = len(tree_df) + surfaces = [ + solid_faces + for (solid_faces, _) in cea.utilities.parallel.vectorize( + process_geometries, num_processes + )( + tree_df["geometry"], + repeat(elevation_map, n), + repeat(range(1), n), + tree_df["height_tc"], + ) + ]Note: This requires passing
configor a process count totree_geometry_generator.
596-597: Use tolerance-based comparison for floating point Z coordinates.Exact float equality
p.z == floor.points[0].zmay fail due to floating point precision errors, rejecting valid near-planar floors.Apply this diff to use a tolerance:
# check if floor is on the XY plane, by subtracting the Z coordinate from point 0 to all points - if not all(p.z == floor.points[0].z for p in floor.points): + if not all(abs(p.z - floor.points[0].z) < 1e-9 for p in floor.points): raise ValueError("The floor polygon must be on the XY plane, all points must have the same Z coordinate.")Alternatively, use
math.iscloseornumpy.iscloseif available:+ import math # check if floor is on the XY plane - if not all(p.z == floor.points[0].z for p in floor.points): + if not all(math.isclose(p.z, floor.points[0].z, abs_tol=1e-9) for p in floor.points): raise ValueError("The floor polygon must be on the XY plane, all points must have the same Z coordinate.")
683-688: Simplify redundant conditional expression.The expression
intersects if intersects in (0, 1) else 0is redundant becauseintersectsis already guaranteed to be0or1based on the return type ofcalc_intersection_face_solid(line 705:Literal[0, 1]).Apply this diff to simplify:
- wall_intersects.extend( - [ - intersects if intersects in (0, 1) else 0 - for _ in range(len(hollowed_facades)) - ] - ) + wall_intersects.extend([intersects] * len(hollowed_facades))
698-698: Simplify redundant conditional expression.The expression
1 if intersects else 0is redundant becauseintersectsis already0or1(line 705:Literal[0, 1]).Apply this diff:
- wall_intersects.append(1 if intersects else 0) + wall_intersects.append(intersects)
711-718: Consider logging OCC warnings instead of suppressing them.The
devnull()context manager suppresses all OCC output, which keeps the console clean but may hide important warnings about geometry issues (e.g., invalid solids, tolerance problems).Consider capturing and logging warnings instead:
import warnings import logging with warnings.catch_warnings(record=True) as w: warnings.simplefilter("always") potentially_intersecting_solid = OCCBrep.from_polygons(potentially_intersecting_solid_faces) state = BRepClass3d_SolidClassifier( potentially_intersecting_solid.occ_shape, gp_Pnt(point.x, point.y, point.z), 1e-6, ) point_in_solid = state.State() == TopAbs_IN if w: for warning in w: logging.debug(f"OCC warning in point-in-solid test: {warning.message}")This preserves diagnostic information while keeping user-facing output clean.
462-469: Consider extractingsafe_floathelper and improving error message.The
safe_floathelper is well-designed for handling numpy scalar conversions, but could be improved:
- The error message could be more specific about the source (e.g., which WWR field contained the complex value).
- If this pattern is used elsewhere, consider extracting to a utility module.
Optional improvements:
- def safe_float(val): + def safe_float(val, field_name="value"): # Convert numpy scalar or complex to float, raise error if complex part is nonzero if hasattr(val, 'real'): if getattr(val, 'imag', 0) != 0: - raise ValueError(f"Cannot convert complex value {val} to float.") + raise ValueError(f"Field '{field_name}' contains a complex value {val} with non-zero imaginary part.") return float(val.real) return float(val)Then use it as:
wwr_west = safe_float(architecture_wwr_df.loc[name, "wwr_west"], "wwr_west")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cea/resources/radiation/geometry_generator.py(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cea/resources/radiation/geometry_generator.py (4)
cea/resources/radiation/building_geometry_radiation.py (3)
BuildingGeometryForRadiation(35-137)from_dict(103-117)save(131-137)cea/utilities/standardize_coordinates.py (3)
crs_to_epsg(68-72)get_lat_lon_projected_shapefile(75-90)get_projected_coordinate_system(55-65)cea/config.py (1)
get_number_of_processes(246-257)cea/inputlocator.py (6)
get_zone_geometry(883-887)get_surroundings_geometry(895-900)get_tree_geometry(878-881)get_solar_radiation_folder(1319-1321)get_terrain(980-982)get_building_architecture(926-931)
⏰ 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)
🔇 Additional comments (1)
cea/resources/radiation/geometry_generator.py (1)
855-855: Past review comment addressed.The code now uses
range(1)which correctly creates a single floor from elevation 0 to height z, avoiding the double-height issue mentioned in the previous review.
This reverts commit f608773.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cea/resources/radiation/geometry_generator.py (2)
462-469: Consider simplifying the complex-value check.The
safe_floathelper appears overly defensive for window-to-wall ratios, which should always be real-valued. If this is specifically to handle numpy scalar edge cases, consider adding a comment explaining the rationale.Apply this diff to add a clarifying comment:
def safe_float(val): - # Convert numpy scalar or complex to float, raise error if complex part is nonzero + # Handle numpy scalars that may expose .real/.imag attributes even for real values if hasattr(val, 'real'): if getattr(val, 'imag', 0) != 0: raise ValueError(f"Cannot convert complex value {val} to float.") return float(val.real) return float(val)
781-799: Consider validating tolerance bounds.The precision calculation at line 795 could produce extreme values for very small tolerances (e.g.,
tolerance=1e-20→decimals=20). Consider adding a reasonable bounds check.Apply this diff to add bounds checking:
raster_points_list = [[float(x), float(y), float(z)] for x, y, z in zip(_x_coords, _y_coords, self.elevation_map[y_index, x_index])] - decimals = int(round(-math.log10(tolerance))) + # Clamp decimals to a reasonable range [0, 15] to avoid extreme precision requirements + decimals = max(0, min(15, int(round(-math.log10(tolerance))))) tin_mesh = Mesh.from_points(raster_points_list) tin_mesh.weld(precision=decimals) return tin_mesh
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cea/resources/radiation/geometry_generator.py(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cea/resources/radiation/geometry_generator.py (3)
cea/resources/radiation/building_geometry_radiation.py (3)
BuildingGeometryForRadiation(35-137)from_dict(103-117)save(131-137)cea/utilities/standardize_coordinates.py (3)
crs_to_epsg(68-72)get_lat_lon_projected_shapefile(75-90)get_projected_coordinate_system(55-65)cea/inputlocator.py (6)
get_zone_geometry(883-887)get_surroundings_geometry(895-900)get_tree_geometry(878-881)get_solar_radiation_folder(1319-1321)get_terrain(980-982)get_building_architecture(926-931)
⏰ 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 (macos-latest)
- GitHub Check: tests (ubuntu-latest)
🔇 Additional comments (13)
cea/resources/radiation/geometry_generator.py (13)
10-58: LGTM! Import structure correctly supports COMPAS migration.The imports properly combine COMPAS geometry primitives with selective OCC functionality for point-in-solid classification, which compas_occ does not yet expose.
61-114: LGTM! Surface classification logic correctly migrated to COMPAS.The function properly uses COMPAS Vector angle calculations to classify surfaces by orientation. The logic for distinguishing facades, roofs, and footprints is correct.
117-147: LGTM! Ray-mesh intersection correctly uses COMPAS primitives.The barycentric interpolation formula (line 143) correctly computes the Z-coordinate of the intersection point. Error handling appropriately raises ValueError when no intersection is found.
148-172: LGTM! Window generation correctly implemented with COMPAS Polygons.The scaling logic (sqrt(wwr)) correctly maintains the area ratio, and the trapezoid generation for hollowed walls is properly implemented.
547-577: LGTM! Solid extrusion correctly uses OCCBrep unions.The function properly uses temporary OCC breps for floor-by-floor extrusion and boolean union, while returning pickle-friendly polygons for multiprocessing. Note that boolean union performance may degrade for buildings with many floors; consider caching or incremental union strategies if performance becomes an issue.
578-613: LGTM! Wall extrusion is well-validated and correctly implemented.The function includes comprehensive type and geometry validation with clear error messages. The wall generation logic correctly handles all edges including the closing edge.
702-723: LGTM! Point-in-solid check correctly uses OCC primitives.The function appropriately uses OCCBrep and OCC.Core for robust point-in-solid classification, which compas_occ does not yet expose. The devnull context properly suppresses OCC warnings.
838-861: Past review addressed: tree extrusion now uses single floor.The code at line 854 now correctly uses
range(1)(which produces[0]), creating a single floor from elevation 0 toheight_tc. This resolves the previous concern about double-height extrusion.Verify that modeling trees as single-floor extrusions (rather than multi-floor structures) aligns with the radiation calculation requirements.
386-395: LGTM! Bounding box computation is correctly implemented.The helper correctly computes the axis-aligned bounding box by flattening all face vertices and finding coordinate extrema.
396-511: LGTM! Zone geometry generation correctly uses polygon-based structures.The function properly constructs
BuildingGeometryForRadiationfrom polygon surfaces, windows, and metadata. Theprocess_facadehelper nicely encapsulates repeated logic.
513-545: LGTM! Footprint elevation sampling correctly uses COMPAS primitives.The function properly samples terrain elevation at the footprint centroid and translates the polygon to the correct elevation.
863-927: LGTM! Main geometry orchestration correctly returns COMPAS types.The function signature and return values properly reflect the migration to COMPAS-based structures (Mesh for terrain, Polygon lists for buildings and trees).
929-1003: LGTM! Example usage correctly demonstrates the COMPAS-based workflow.The
__main__block properly shows how to use the refactored geometry generation pipeline. The commented visualization code provides useful examples for debugging.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cea/resources/radiation/geometry_generator.py (1)
624-709: Minor redundancy in intersection flag handling.The logic correctly generates windows and walls based on WWR and intersection status. However, line 694's conditional
intersects if intersects in (0, 1) else 0is redundant sinceintersectsis already constrained to 0 or 1 by the logic above (lines 669-673). This doesn't affect correctness, just adds unnecessary defensive checks.If you want to simplify, apply this diff:
wall_intersects.extend( - [ - intersects if intersects in (0, 1) else 0 - for _ in range(len(hollowed_facades)) - ] + [intersects] * len(hollowed_facades) )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cea/resources/radiation/geometry_generator.py(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cea/resources/radiation/geometry_generator.py (4)
cea/resources/radiation/building_geometry_radiation.py (3)
BuildingGeometryForRadiation(35-137)from_dict(103-117)save(131-137)cea/utilities/standardize_coordinates.py (3)
crs_to_epsg(68-72)get_lat_lon_projected_shapefile(75-90)get_projected_coordinate_system(55-65)cea/config.py (2)
Configuration(35-277)get_number_of_processes(246-257)cea/inputlocator.py (7)
InputLocator(20-1505)get_zone_geometry(883-887)get_surroundings_geometry(895-900)get_tree_geometry(878-881)get_solar_radiation_folder(1319-1321)get_terrain(980-982)get_building_architecture(926-931)
⏰ 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 (16)
cea/resources/radiation/geometry_generator.py (16)
64-117: LGTM!The surface type identification logic correctly classifies polygons by angle to vertical (facades at 45°-135°, roofs <45°, footprints >135°) and then subdivides facades by orientation using signed horizontal angle. Boundary conditions are handled appropriately.
120-149: LGTM!The ray-mesh intersection correctly uses COMPAS's
intersection_ray_meshand performs barycentric interpolation to compute the precise Z-coordinate of the hit point. The function properly raises an error when no intersection is found.
151-174: LGTM!The window generation correctly scales the surface by
sqrt(wwr)to achieve the target area ratio, then creates four trapezoid wall segments around the window. This is a cleaner approach than the previous 8-triangle subdivision.
176-223: LGTM!The function correctly parallelizes building solid generation and returns pickle-friendly polygon lists. The use of
range_floorsproperly handles void decks.
225-256: LGTM!The function cleanly separates elevation sampling from floor extrusion. The implementation matches the documented behavior.
258-281: LGTM!The function correctly generates surrounding building geometry without windows (as expected for context buildings) and persists it using the new
BuildingGeometryForRadiationdataclass.
284-368: LGTM!The function correctly orchestrates parallel 3D geometry generation for both zone and surrounding buildings, properly handling empty surroundings and adjacency settings.
379-397: LGTM!The proximity check uses southwest corner distance with a 100m threshold, which is a reasonable approximation for adjacency detection. The new
faces_bounding_boxhelper correctly computes axis-aligned bounding boxes from polygon lists.
399-513: LGTM!The function correctly generates full radiation geometry including windows, walls, roofs, and undersides. The
safe_floathelper provides robust handling of numpy scalars, and theprocess_facadeinner function nicely eliminates code duplication across orientations.
516-547: LGTM!The function correctly samples terrain elevation at the footprint centroid and translates the entire footprint polygon to that elevation. The approach is sound for placing buildings on terrain.
550-622: LGTM!The solid extrusion correctly handles slab (first floor), walls (all floors), and roof (last floor). The
from_floor_extrude_wallshelper includes robust input validation (type checking, positive height, coplanar points) and properly closes the wall loop.
712-732: LGTM!The function correctly uses OCC's
BRepClass3d_SolidClassifierfor robust point-in-solid testing. This is the appropriate approach given that compas_occ doesn't expose a higher-level API for this operation.
791-808: LGTM!The TIN generation correctly converts the elevation raster to a COMPAS mesh and welds vertices to remove duplicates. The precision calculation from tolerance is appropriate.
848-870: Past review comment is now resolved.The past review suggested changing
(0, 1)torange(1)to avoid double-height tree extrusion. The current code already usesrange(1)(line 864), which correctly generates trees with heightz(slab at elevation 0, walls from 0 to z, roof at z).
873-936: LGTM!The main geometry function correctly orchestrates the full workflow: coordinate standardization, terrain generation, building 3D conversion, and optional tree geometry. The return types match the updated signature.
939-1012: LGTM!The main block correctly sets up test inputs and calls
geometry_main. The commented-out visualization code is appropriate for a module that will be imported rather than run standalone.
…xtrusions, and avoid using occ_brep.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cea/resources/radiation/geometry_generator.py (2)
59-60: Tolerance comment has minor arithmetic error.The comment states "1e-10 relative is about 1e-3 absolute" but the calculation 1e-10 × 4e7 = 4e-3 (4 mm), not 1e-3 (1 mm). This was flagged in a previous review.
844-866: Tree extrusion height issue already addressed.A previous review flagged that using floor indices
(0, 1)would double the tree height. The current code usesrange(1), which produces[0](a single floor), correctly creating a tree of heightzwithout doubling.
🧹 Nitpick comments (3)
cea/resources/radiation/geometry_generator.py (3)
463-474: Consider simplifyingsafe_floatif data is guaranteed numeric.This function handles numpy scalars and complex numbers, but window-to-wall ratios from
architecture_wwr_dfshould always be real numeric values. The complex-number check is overly defensive unless you've observed data-quality issues.If you're confident the data is always real-valued, simplify to:
- def safe_float(val): - # Convert numpy scalar or complex to float, raise error if complex part is nonzero - if hasattr(val, 'real'): - if getattr(val, 'imag', 0) != 0: - raise ValueError(f"Cannot convert complex value {val} to float.") - return float(val.real) - return float(val) + def safe_float(val): + return float(val)Otherwise, retain the defensive checks.
690-696: Simplify redundant intersection-value condition.Since
calc_intersection_face_solidreturnsLiteral[0, 1],intersectsis always either 0 or 1. The conditionintersects if intersects in (0, 1) else 0is redundant.Simplify to:
- wall_intersects.extend( - [ - intersects if intersects in (0, 1) else 0 - for _ in range(len(hollowed_facades)) - ] - ) + wall_intersects.extend([intersects] * len(hollowed_facades))
706-706: Simplify redundant intersection-value condition.Since
intersectsis already 0 or 1, the ternary1 if intersects else 0is redundant.Simplify to:
- wall_intersects.append(1 if intersects else 0) + wall_intersects.append(intersects)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cea/resources/radiation/geometry_generator.py(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cea/resources/radiation/geometry_generator.py (3)
cea/resources/radiation/building_geometry_radiation.py (3)
BuildingGeometryForRadiation(35-137)from_dict(103-117)save(131-137)cea/utilities/standardize_coordinates.py (3)
crs_to_epsg(68-72)get_lat_lon_projected_shapefile(75-90)get_projected_coordinate_system(55-65)cea/inputlocator.py (6)
get_zone_geometry(883-887)get_surroundings_geometry(895-900)get_tree_geometry(878-881)get_solar_radiation_folder(1319-1321)get_terrain(980-982)get_building_architecture(926-931)
⏰ 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 (ubuntu-latest)
- GitHub Check: tests (windows-latest)
🔇 Additional comments (2)
cea/resources/radiation/geometry_generator.py (2)
787-804: Verify thatMesh.from_pointsproduces a valid TIN.The docstring states this method generates a triangulated irregular network (TIN), and the code calls
Mesh.from_points(raster_points_list). Confirm thatcompas.datastructures.Mesh.from_pointsperforms a Delaunay triangulation (or equivalent) rather than simply storing a point cloud, to ensure a proper TIN is created.Based on learnings, the codebase previously used py4design for TIN generation. If
Mesh.from_pointsdoes not triangulate, you may need to use a COMPAS-specific triangulation method orcompas_libiglfor meshing.
711-728: Revise inline comment in calc_intersection_face_solid
The face list always begins with the slab/footprint (as appended first by calc_solid), so the index assumption is safe. However, the comment on the second check (“then check if point is inside the bounding box of footprint”) is misleading—update it to something like:# then check if point is inside the footprint polygon in the XY plane
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
cea/resources/radiation/sensor_area_calculator.py (3)
31-50: Speed up cell clipping with prepared geometry + intersects precheck.Using shapely.prepared.prep and intersects() avoids many expensive intersections.
Minimal changes:
- Import once:
+from shapely.prepared import prep
- Prepare polygon and precheck per cell:
- poly_s = SPolygon([(p.x, p.y) for p in poly_local.points]) + poly_s = SPolygon([(p.x, p.y) for p in poly_local.points]) + poly_prepared = prep(poly_s) @@ - inter = sbox(x0, y0, x1, y1).intersection(poly_s) + cell = sbox(x0, y0, x1, y1) + if not poly_prepared.intersects(cell): + continue + inter = cell.intersection(poly_s)
46-49: Interiors (holes) are ignored; confirm assumption or handle holes.You use only g.exterior; polygons with holes will overcount area and misplace centroids. If faces are guaranteed hole‑free (windows handled separately), add a comment asserting this; otherwise, decompose holes (e.g., shapely polygon without interiors) before converting.
12-17: Make frame construction robust to nearly colinear first 3 points.Frame.from_points(face.points[:3]) can fail if points are near‑colinear. Prefer a plane/frame derived from the polygon (e.g., face.plane → Frame.from_plane) if available, or pick a non‑colinear triple.
Based on learnings
cea/resources/radiation/daysim.py (3)
156-169: Avoid magic offset; make it a named constant or parameter.0.01 m translate is a hidden assumption. Define SENSOR_OFFSET = 0.01 (or pull from tolerance) and reference it for clarity/tuning.
- moved_face: Polygon = face.translated(normal.scaled(0.01)) + SENSOR_OFFSET = 0.01 # meters; small offset to avoid coplanar artefacts + moved_face: Polygon = face.translated(normal.scaled(SENSOR_OFFSET))
191-196: Typo: renameinteresection_listtointersection_list.Spelling inconsistency hurts readability and IDE tooling.
- face_list, orientation_list, normals_list, interesection_list = ( + face_list, orientation_list, normals_list, intersection_list = ( building_geometry.group(srf_type) ) - for orientation, normal, face, intersection in zip( - orientation_list, normals_list, face_list, interesection_list + for orientation, normal, face, intersection in zip( + orientation_list, normals_list, face_list, intersection_list ):
45-48: GridSize should be float-typed.Grid sizes are often fractional; declaring as int misleads type hints.
-class GridSize(NamedTuple): - roof: int - walls: int +class GridSize(NamedTuple): + roof: float + walls: float
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cea/resources/radiation/daysim.py(7 hunks)cea/resources/radiation/sensor_area_calculator.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cea/resources/radiation/daysim.py (2)
cea/resources/radiation/building_geometry_radiation.py (3)
BuildingGeometryForRadiation(35-137)group(65-92)load(120-129)cea/resources/radiation/sensor_area_calculator.py (1)
partition_polygon_by_grid(9-50)
⏰ 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 (2)
cea/resources/radiation/daysim.py (2)
230-246: Verify geometry pickle path.load() expects a file path. os.path.join(geometry_pickle_dir, 'zone', building_name) may point to a directory or lack an extension. Confirm actual on‑disk path (e.g., .../zone/.pkl) and adjust accordingly.
300-309: Confirm Daysim writer accepts compas Points/Vectors.create_sensor_input_file likely expects numeric (x, y, z) tuples. If it doesn’t accept compas types, convert first.
If needed, change to:
- daysim_project.create_sensor_input_file(sensors_coords_zone, sensors_dir_zone) + coords = [(p.x, p.y, p.z) for p in sensors_coords_zone] + dirs = [(v.x, v.y, v.z) for v in sensors_dir_zone] + daysim_project.create_sensor_input_file(coords, dirs)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cea/resources/radiation/geometry_generator.py (1)
57-58: Arithmetic error in tolerance comment already noted.The past review correctly identified that
1e-10 × 4e7 = 4e-3(4 mm), not 1e-3 (1 mm). The tolerance value itself is reasonable for building geometry.
🧹 Nitpick comments (2)
cea/resources/radiation/geometry_generator.py (2)
703-708: Simplify redundant ternary in list comprehension.The ternary
intersects if intersects in (0, 1) else 0is redundant sinceintersectsis already typed asLiteral[0, 1]from the surrounding logic (line 680). The condition will always evaluate to the first branch.Apply this diff to simplify:
- wall_intersects.extend( - [ - intersects if intersects in (0, 1) else 0 - for _ in range(len(hollowed_facades)) - ] - ) + wall_intersects.extend([intersects] * len(hollowed_facades))
723-740: Document footprint index assumption.Line 737 assumes
potentially_intersecting_solid_faces[0]is the footprint polygon for the 2D containment check. While this is correct given the construction incalc_solid, the assumption is implicit and could lead to bugs if the face ordering changes.Consider adding a docstring note or assertion:
def calc_intersection_point_solid_faces( potentially_intersecting_solid_faces: list[Polygon], point: Point ) -> Literal[0, 1]: """ Check if a point is inside a solid defined by its exterior faces. Because the solids are vertical extrusions of 2D footprints, we can first check if the point is within the z-bounds of the solid, then check if the point is within the 2D footprint polygon. If both checks pass, the point is inside the solid. + + :param potentially_intersecting_solid_faces: Exterior polygons where the first element MUST be the footprint. """
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cea/resources/radiation/geometry_generator.py(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cea/resources/radiation/geometry_generator.py (3)
cea/resources/radiation/building_geometry_radiation.py (3)
BuildingGeometryForRadiation(35-137)from_dict(103-117)save(131-137)cea/utilities/standardize_coordinates.py (3)
crs_to_epsg(68-72)get_lat_lon_projected_shapefile(75-90)get_projected_coordinate_system(55-65)cea/inputlocator.py (5)
get_zone_geometry(878-882)get_surroundings_geometry(890-895)get_tree_geometry(873-876)get_solar_radiation_folder(1314-1316)get_terrain(975-977)
⏰ 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 (ubuntu-latest)
- GitHub Check: tests (windows-latest)
🔇 Additional comments (5)
cea/resources/radiation/geometry_generator.py (5)
60-113: LGTM: Surface classification migrated to COMPAS.The function correctly identifies facades, roofs, and footprints using COMPAS Vector operations. The signed angle calculation with right-hand rule properly orients surfaces to cardinal directions.
116-146: LGTM: Ray-mesh intersection correctly implemented.The function properly uses COMPAS's
intersection_ray_meshand correctly interpolates the intersection point's z-coordinate using barycentric coordinates (w = 1 - u - v).
147-171: LGTM: Window generation with trapezoid walls.The function correctly scales the surface by
sqrt(wwr)to preserve area ratio and generates trapezoid walls between window and surface edges. This replaces the prior triangulation approach as noted in the PR objectives.
474-481: Verify if complex WWR values are expected in the data.The
safe_floathelper defensively handles complex numbers from the architecture DataFrame, but complex-valued window-to-wall ratios seem unlikely in practice. If complex values appear, this might indicate a data quality issue upstream (e.g., incorrect parsing or computation) rather than an expected scenario.Consider verifying whether complex numbers can legitimately appear in
architecture_wwr_dfor if this is defensive coding for an edge case that should be caught earlier in the pipeline.
856-878: LGTM: Tree height issue from past review is fixed.The tree geometry generation now correctly uses
range(1)instead of(0, 1), which produces a single floor and avoids the double-height issue identified in the previous review.Based on past review comments.
|
Previously, we noticed that the building might have very different irradiation results if they are shaded (see the results in
@reyery seems that the master with world XY, or generating sensors rotating from local grid seems to be beneficial. Should I always rotate the sensor grid before generating sensors? |
…s than 1e-4 in partition_polygon_by_grid function.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cea/resources/radiation/sensor_area_calculator.py (1)
23-27: Guard against non‑positive grid sizes (ZeroDivision risk).If
grid_dxorgrid_dyare ≤ 0, lines 23–24 will raiseZeroDivisionError. This critical issue was flagged in a previous review and remains unaddressed.Apply this diff:
def partition_polygon_by_grid( face: Polygon, grid_dx: float, grid_dy: float ) -> list[Polygon]: + if grid_dx <= 0 or grid_dy <= 0: + raise ValueError(f"grid_dx and grid_dy must be > 0 (got {grid_dx}, {grid_dy})") f = Frame.from_points(face.points[0], face.points[1], face.points[2])
🧹 Nitpick comments (2)
cea/resources/radiation/sensor_area_calculator.py (2)
47-47: Consider making the tiny patch threshold configurable.The hardcoded threshold
1e-4filters out small patches but may not be appropriate for all grid resolutions or polygon scales. Consider adding a parameter (e.g.,min_patch_area: float = 1e-4) or documenting why this value was chosen.Example:
def partition_polygon_by_grid( - face: Polygon, grid_dx: float, grid_dy: float + face: Polygon, grid_dx: float, grid_dy: float, min_patch_area: float = 1e-4 ) -> list[Polygon]: # ... for g in geoms: - if g.area < 1e-4: # avoid tiny patches + if g.area < min_patch_area: continue
55-128: Consider moving demo code to a separate test or example file.The demonstration code under
if __name__ == "__main__"is useful for testing and debugging but might be better suited in a separate test file or examples directory. This keeps the production module focused on its core functionality.However, if this module is primarily internal and the team prefers having quick visual tests alongside the implementation, the current approach is acceptable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cea/resources/radiation/sensor_area_calculator.py(1 hunks)
⏰ 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)
| def partition_polygon_by_grid( | ||
| face: Polygon, grid_dx: float, grid_dy: float | ||
| ) -> list[Polygon]: |
There was a problem hiding this comment.
Add input validation for face geometry.
The function assumes face has at least 3 points. If face.points has fewer than 3 elements, line 12 will raise an IndexError when attempting to construct the frame.
Add a guard at the function start:
def partition_polygon_by_grid(
face: Polygon, grid_dx: float, grid_dy: float
) -> list[Polygon]:
+ if len(face.points) < 3:
+ raise ValueError(f"Face must have at least 3 points, got {len(face.points)}")
f = Frame.from_points(face.points[0], face.points[1], face.points[2])📝 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.
| def partition_polygon_by_grid( | |
| face: Polygon, grid_dx: float, grid_dy: float | |
| ) -> list[Polygon]: | |
| def partition_polygon_by_grid( | |
| face: Polygon, grid_dx: float, grid_dy: float | |
| ) -> list[Polygon]: | |
| if len(face.points) < 3: | |
| raise ValueError(f"Face must have at least 3 points, got {len(face.points)}") | |
| f = Frame.from_points(face.points[0], face.points[1], face.points[2]) | |
| # … rest of function unchanged … |
🤖 Prompt for AI Agents
In cea/resources/radiation/sensor_area_calculator.py around lines 9 to 11, the
function partition_polygon_by_grid assumes face has at least 3 points and will
IndexError when constructing the frame if face.points has fewer than 3 elements;
add an early guard that verifies face is not None, has a points attribute (or
behaves like an iterable of coordinates) and that len(face.points) >= 3, and if
the check fails raise a clear ValueError (e.g. "face must contain at least 3
points") so callers get a descriptive error instead of an IndexError.
|
@yiqiaowang-arch Interesting results, thanks for the comparison. It seems that this PR creates lesser sensor points compared to master for (1, 1) but still giving the about the same results. Is this mainly due to sensors on edges behaviour in master? |
I believe so, the master should have more sensors on edge (or close to edge) than this PR. |
|
I have a feeling that this might be a very specific case where more sensors on edge benefits this particular shading pattern (i.e., floating building right above, only areas close to edge have higher irradiation). So even doing the rotation strategy, as long as it does not have enough sensors on the edges, the results will not match the high resolution results. The only way to "fix" this would be to always line the edges with sensors, but this might be hard to reason with since it might introduce logic to solve what I would consider an edge case. So on my end the 2 questions to be considered is:
|
Answer to the first question: we could add additional sensors around the edges, each taking up a circle as its representing area; and subtract the neighboring sensor areas by the intersecting area with the circles. But before that, I could try mimicking the py4design behavior first. For the second question: This happens when there's a building shaded heavily by another building directly from the top, but I cannot think of other cases. |
2 participants
this PR replaces the out-of-service py4design package used in the radiation calculation with a currently maintained geometry framework, COMPAS (https://compas.dev/), including
compasandcompas_libigl. For debugging purposes, one would also need to installcompas_viewer.Changes:
compas.geometry.Polygonto define building geometries.daysimand inradiance, usecompas.geometry.Pointandcompas.geometry.Vectorto define sensor points and its direction.BuildingGeometryintoBuildingGeometryForRadiationto avoid naming duplication with theBuildingGeometryclass for thermal calculation. Also changed it into adataclassfor better type hinting.sensor_area_calculator.Comparison and consequences of sensor point generation methods:
Currently, there are a few problems that need to be addressed:
sensor_area_calculatoris unstable and need more testing.Summary by CodeRabbit
New Features
Refactor
Chores