replace py4design in radiation_daysim with compas#3919
replace py4design in radiation_daysim with compas#3919yiqiaowang-arch wants to merge 46 commits intoarchitecture-building-systems:masterfrom
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 partitioner for sensor tiling, and migrates Daysim, Radiance, geometry-generation, and CRAX 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 Potential focus areas for review:
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)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-11-26T23:07:30.367ZApplied to files:
📚 Learning: 2025-11-26T23:07:30.367ZApplied to files:
🧬 Code graph analysis (1)cea/resources/radiation/main.py (2)
⏰ 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)
🔇 Additional comments (3)
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: 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. |
There was a problem hiding this comment.
Pull request overview
This PR migrates CEA's radiation calculation module from the deprecated py4design library to the actively maintained COMPAS framework (v2.14.1+). The refactoring introduces a more robust, type-safe geometry model while maintaining functional equivalence.
Key changes:
- Replaced OCC brep geometry with COMPAS
Polygon,Point, andVectortypes for radiation calculations - Refactored
BuildingGeometryintoBuildingGeometryForRadiationdataclass with better type hinting - Implemented new sensor point distribution algorithm using local coordinate frames instead of world XY coordinates
- Simplified window generation from 9-part triangulation to 4-trapezoid decomposition
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Adds compas and compas-libigl>=0.7.6,<0.8 dependencies |
| pixi.lock | Updates lock file with new dependencies; removes h3-py/timezonefinder conda packages, adds pypi equivalents |
| building_geometry_radiation.py | New dataclass-based geometry container with type-safe serialization |
| sensor_area_calculator.py | New grid partitioning utility using local coordinate frames for sensor placement |
| geometry_generator.py | Complete refactor to COMPAS types; replaces OCC intersection/boolean ops with COMPAS mesh operations |
| radiance.py | Updates RadSurface to accept Polygon instead of OCC faces; updates type annotations |
| daysim.py | Refactors sensor generation to use COMPAS types and new partitioning algorithm |
| main.py | Updates import path for renamed BuildingGeometryForRadiation class |
| radiationCRAX/main.py | Updates import path for renamed BuildingGeometryForRadiation class |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else: | ||
| continue | ||
| for g in geoms: | ||
| if g.area < 1e-4: # avoid tiny patches |
There was a problem hiding this comment.
[nitpick] Missing space after # in inline comment. Should be # avoid tiny patches instead of # avoid tiny patches.
| if g.area < 1e-4: # avoid tiny patches | |
| if g.area < 1e-4: # avoid tiny patches |
| # "windows_w": "windows_west_kW", | ||
| # }, inplace=True) | ||
| # | ||
| # # Add surface area | ||
| # building_metadata = pd.read_csv(locator.get_radiation_metadata(building_name)) | ||
| # surface_area = building_metadata.groupby(['TYPE', 'orientation'])['AREA_m2'].sum() | ||
| # existing_surface_directions = set() | ||
| # for index, value in surface_area.items(): | ||
| # surface_direction = f"{index[0]}_{index[1]}" | ||
| # existing_surface_directions.add(surface_direction) | ||
| # radiation_file[f"{surface_direction}_m2"] = round(value, 2) | ||
| # # Add missing surfaces to output | ||
| # missing_surface_directions = SURFACE_DIRECTION_LABELS - existing_surface_directions | ||
| # for surface_direction in missing_surface_directions: | ||
| # radiation_file[f"{surface_direction}_kW"] = 0.0 | ||
| # radiation_file[f"{surface_direction}_m2"] = 0.0 | ||
| # | ||
| # radiation_file.set_index("date").to_csv(radiation_file_path) | ||
|
|
There was a problem hiding this comment.
This comment appears to contain commented-out code.
| # "windows_w": "windows_west_kW", | |
| # }, inplace=True) | |
| # | |
| # # Add surface area | |
| # building_metadata = pd.read_csv(locator.get_radiation_metadata(building_name)) | |
| # surface_area = building_metadata.groupby(['TYPE', 'orientation'])['AREA_m2'].sum() | |
| # existing_surface_directions = set() | |
| # for index, value in surface_area.items(): | |
| # surface_direction = f"{index[0]}_{index[1]}" | |
| # existing_surface_directions.add(surface_direction) | |
| # radiation_file[f"{surface_direction}_m2"] = round(value, 2) | |
| # # Add missing surfaces to output | |
| # missing_surface_directions = SURFACE_DIRECTION_LABELS - existing_surface_directions | |
| # for surface_direction in missing_surface_directions: | |
| # radiation_file[f"{surface_direction}_kW"] = 0.0 | |
| # radiation_file[f"{surface_direction}_m2"] = 0.0 | |
| # | |
| # radiation_file.set_index("date").to_csv(radiation_file_path) |
|
|
||
| Currently Intel Macs are not supported. | ||
| """ | ||
| return |
There was a problem hiding this comment.
This statement is unreachable.
| return |
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
✏️ Tip: You can customize this high-level summary in your review settings.