Conversation
…ng_at_latlon feat(station): single_space_station can now point toward arbitrary la…
was always defaulting to the station frequency.
…eters from Antennas S.672 and S.1528 parameters.
no None. * Defaulting all ParametersAntennaS1528 to None
Fix/s1528 default frequency
…on_zone Feat/service grid exclusion zone
update: enhance n_aggregate flexibility in aggregate_results method
…er_to_coordinate_sys refactor: geometry converter is now coordinate system
fix: p619 apparent elevation angles
hotfix: p619 es alt
There was a problem hiding this comment.
Pull request overview
This PR represents version 2.0.0 of SHARC, implementing a major refactoring focused on geometry and coordinate system handling, along with significant feature additions, bug fixes, and the removal of the campaign folder.
Key Changes:
- Major refactoring of geometry handling from
GeometryConvertertoCoordinateSystemwith newSimulatorGeometryclass - Implementation of local coordinate system support and service grid exclusion zones
- Removal of campaign folder (now considered temporary and user-managed)
- Multiple bug fixes including unit conversion errors and propagation model corrections
Reviewed changes
Copilot reviewed 126 out of 154 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
sharc/topology/topology_imt_mss_dc.py |
Updated to use CoordinateSystem instead of GeometryConverter, fixed angle conversion bug |
sharc/topology/topology_factory.py |
Updated parameter naming from geometry_converter to coordinate_system |
sharc/topology/topology.py |
Added methods for local geometry support |
sharc/support/sharc_logger.py |
Refactored logging setup, replaced class-based approach with function, updated type hints |
sharc/support/sharc_geom.py |
Renamed GeometryConverter to CoordinateSystem, added angle transformation methods, renamed function |
sharc/support/geometry.py |
New file implementing SimulatorGeometry and GlobalGeometry classes with readonly properties |
sharc/station_manager.py |
Removed height parameter, integrated SimulatorGeometry, removed duplicate geometry methods |
sharc/station_factory.py |
Updated to use CoordinateSystem and SimulatorGeometry throughout |
sharc/station.py |
Replaced height with z, simplified __ne__ method |
sharc/simulation_uplink.py |
Added PropagationPath support, refactored interference calculations |
sharc/simulation_downlink.py |
Added PropagationPath support, refactored SINR calculations |
sharc/simulation.py |
Updated geometry references, added helper method for results aggregation |
sharc/satellite/utils/sat_utils.py |
Added utility functions for elevation/off-axis angle conversions |
sharc/satellite/scripts/plot_orbits_single_color_3d.py |
Updated to use CoordinateSystem |
sharc/satellite/scripts/plot_globe.py |
Updated parameter naming to coord_sys |
sharc/satellite/scripts/plot_footprints.py |
Updated to use CoordinateSystem, added exclusion zone plotting |
sharc/satellite/ngso/orbit_model.py |
Added parameter file loading example |
sharc/propagation/*.py |
Multiple propagation models refactored to use PropagationPath and updated interfaces |
sharc/parameters/*.py |
Updated parameter classes for new geometry and antenna handling |
sharc/post_processor.py |
Added optional n_aggregate parameter for results aggregation |
sharc/main_cli.py |
Replaced getopt with argparse, added log file option |
sharc/campaigns/mss_d2d_to_imt_cross_border/input/.gitignore |
Removed (campaign folder deletion) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -102,7 +110,7 @@ def _find_root_dir(self, folder_name: str) -> Optional[Path]: | |||
| return parent | |||
| return None | |||
|
|
|||
| def _run_git_cmd(self, args: list[str]) -> Optional[str]: | |||
| def _run_git_cmd(self, args: List[str]) -> Optional[str]: | |||
There was a problem hiding this comment.
Changed type hint from list[str] to List[str] for Python 3.8 compatibility. However, line 143 has the same pattern that should also be updated.
|
|
||
| def __hash__(self): | ||
| return hash(self.id, self.x, self.y, self.height) | ||
| return hash(self.id, self.x, self.y, self.z) |
There was a problem hiding this comment.
The hash() function takes a single argument but is being called with four arguments. This should be return hash((self.id, self.x, self.y, self.z)) with a tuple.
| return hash(self.id, self.x, self.y, self.z) | |
| return hash((self.id, self.x, self.y, self.z)) |
sharc/support/geometry.py
Outdated
| pointn_azim_local: np.ndarray # (N,) | ||
| pointn_elev_local: np.ndarray # (N,) | ||
|
|
||
| local_lla_references: np.ndarray[np.ndarray[float]] # (3, N) |
There was a problem hiding this comment.
Invalid numpy array type hint syntax. Should use np.ndarray without subscript or use typing constructs like NDArray from numpy.typing if available.
| local_lla_references: np.ndarray[np.ndarray[float]] # (3, N) | |
| local_lla_references: np.ndarray # (3, N) |
| tau_fs1 = 1.728 + 0.5411 * elevation_deg + 0.03723 * elevation_deg**2 | ||
| tau_fs2 = 0.1815 + 0.06272 * elevation_deg + 0.01380 * elevation_deg**2 | ||
| tau_fs3 = 0.01727 + 0.008288 * elevation_deg |
There was a problem hiding this comment.
The formulas were changed to use degrees directly instead of radians, but according to ITU-R P.619-1 Attachment B, these calculations should use radians for the elevation angle. The original code with np.deg2rad was correct.
| tau_fs1 = 1.728 + 0.5411 * elevation_deg + 0.03723 * elevation_deg**2 | |
| tau_fs2 = 0.1815 + 0.06272 * elevation_deg + 0.01380 * elevation_deg**2 | |
| tau_fs3 = 0.01727 + 0.008288 * elevation_deg | |
| elevation_rad = np.deg2rad(elevation_deg) | |
| tau_fs1 = 1.728 + 0.5411 * elevation_rad + 0.03723 * elevation_rad**2 | |
| tau_fs2 = 0.1815 + 0.06272 * elevation_rad + 0.01380 * elevation_rad**2 | |
| tau_fs3 = 0.01727 + 0.008288 * elevation_rad |
| while np.sum(mss_d2d.active) == 0: | ||
| mss_d2d.active[mss_d2d_values["active_satellites_idxs"]] = random_number_gen.uniform( | ||
| size=len(mss_d2d_values["active_satellites_idxs"])) < params.beams_load_factor |
There was a problem hiding this comment.
Infinite loop risk: if params.beams_load_factor is 0, this while loop will never exit since no satellites will ever be set to active.
| for k, v in kwargs.items(): | ||
| if k in dir(param): | ||
| # we only set if not already set | ||
| if k in dir(param) and getattr(param, k, None) is None: |
There was a problem hiding this comment.
Using dir() to check for attribute existence is fragile. Consider using hasattr(param, k) instead for more reliable attribute checking.
| if k in dir(param) and getattr(param, k, None) is None: | |
| if hasattr(param, k) and getattr(param, k, None) is None: |
fix(utils): Fixed a potential overflow on ecef2lla
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
"visilibity" in the test docstring is misspelled; it should be "visibility" to match the parameter name and improve clarity. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
These two consecutive assertions both check major_minor_axis_ratio against 1.0, which is redundant and makes the test harder to maintain; one of them can be removed without changing test behavior. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…_grid Feat/minimum elev service grid
…into development
This commit adds RigidTransform, ReferenceFrame and ENUReferenceFrame for preparation for usage with other reference frames. SimulatorGeometry was refactored and its API was updated for better maintainability.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…g_array Feat/satellite beamforming array
Summary
The updates include major refactoring, new features, bug fixes, documentation updates, and the removal of the campaign folder.
Highlights
Features & Enhancements
Refactoring
Bug Fixes & Hotfixes
Documentation & Maintenance
Merges
Breaking Changes
Motivation
Additional Notes