-
Notifications
You must be signed in to change notification settings - Fork 0
Decrease dependency on elevation / azimuth #41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Reduces reliance on elevation/azimuth-derived coordinates and expands per-telescope feature set by adding channel-count features.
Changes:
- Removed
tel_shower_x/tel_shower_yfrom the feature set and related synthetic-feature computation. - Added
ntubesandnlowgainas telescope features, including clip intervals and log10 transforms. - Changed
array_footprintcomputation to use telescope ground coordinates (rather than shower-plane coordinates), with special handling for low-multiplicity events.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/eventdisplay_ml/features.py |
Updates the declared feature list and clip intervals (adds ntubes/nlowgain, removes tel_shower_*). |
src/eventdisplay_ml/data_processing.py |
Removes tel_shower_* synthesis, applies log10 to new count features, and rewrites array_footprint to use ground coordinates. |
Comments suppressed due to low confidence (2)
src/eventdisplay_ml/data_processing.py:429
- After removing tel_shower_x/y support, make_relative_coord_columns no longer uses the pointing angles, but this call site still computes/passes np.radians(ArrayPointing*). Consider removing the unused parameters from _make_relative_coord_columns and skipping these per-event conversions to keep the API aligned and avoid unnecessary work.
if var in ("tel_rel_x", "tel_rel_y") and tel_config:
_logger.info(f"Computing synthetic feature: {var}")
flat_features.update(
_make_relative_coord_columns(
var,
src/eventdisplay_ml/data_processing.py:898
- _calculate_array_footprint sizes lookup_x/lookup_y from the max telescope id present in tel_list_matrix, but then writes all tel_config telescope IDs into those arrays. If tel_config contains IDs larger than any ID appearing in tel_list_matrix (or tel_list_matrix is all-NaN), this will raise IndexError. Size the lookup arrays using tel_config["max_tel_id"] (or guard the assignments).
# Pre-map all telescope positions to a dense array aligned with tel_list_matrix IDs
max_id = int(np.nanmax(tel_list_matrix)) if np.any(~np.isnan(tel_list_matrix)) else 0
lookup_x = np.zeros(max_id + 1)
lookup_y = np.zeros(max_id + 1)
for tid, tx, ty in zip(tel_config["tel_ids"], tel_config["tel_x"], tel_config["tel_y"]):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Closes #40