Skip to content
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

Muon parameters update #2670

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Muon parameters update #2670

wants to merge 20 commits into from

Conversation

jstvdk
Copy link
Contributor

@jstvdk jstvdk commented Dec 10, 2024

I added new functionality to the muon analysis as described in the Issue #2665 , specifically

  • new fields to the MuonParametersContainer
  • new methods to the ctapipe/image/muon/features.py
  • new tests for above mentioned methods in ctapipe/image/muon/tests/test_muon_features.py
  • additional code block in ctapipe/image/muon/processor.py to process and save new muon parameters

Added changes mainly follow existing code in lstchain/image/muon/muon_analysis.py, with one exception:

  • I redefined the variable num_pixels_in_ring - now it takes into account only pixels that survived the cleaning, contrary to what currently implemented in lstchain, where this variable contains all pixels inside the geometrical area of the ring.

Closes #2665

@mexanick mexanick marked this pull request as draft December 10, 2024 10:06
@jstvdk jstvdk self-assigned this Dec 10, 2024
Copy link
Member

@maxnoe maxnoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jstvdk

Thanks a lot for preparing this. This looks like a good starting point. I have a couple of first comments mainly about the names of the fields and their descriptions.

I will do second round on more technical issues of the code later.

kosack
kosack previously requested changes Dec 18, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

1 similar comment

This comment has been minimized.

@mexanick mexanick assigned mexanick and unassigned jstvdk Jan 23, 2025
@mexanick mexanick added the module:calib issues related to ctapipe.calib label Jan 23, 2025
@mexanick mexanick force-pushed the muon-parameters-update branch from c55cd9b to 29c3e83 Compare March 10, 2025 18:06
@mexanick mexanick requested review from kosack and maxnoe March 10, 2025 18:09
@mexanick mexanick marked this pull request as ready for review March 10, 2025 18:09
@mexanick
Copy link
Contributor

docs are failing due to an issue with the type hint resolution with a few similar warnings:

/home/runner/work/ctapipe/ctapipe/src/ctapipe/image/muon/features.py:docstring of ctapipe.image.muon.features.intensity_ratio_inside_ring:1: WARNING: py:class reference target not found: ctapipe.containers.MuonRingContainer [ref.class]

@maxnoe any idea how to fix it (apart from adding more ctapipe-related stuff in nitpick_ignore_regex?

@maxnoe
Copy link
Member

maxnoe commented Mar 10, 2025

Is the MuonRingContainer present in __all__ in containers.py?

This comment has been minimized.

maxnoe
maxnoe previously approved these changes Mar 11, 2025

This comment has been minimized.

Copy link

Passed

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage 99.10% Coverage (93.70% Estimated after merge)
  • Duplications 0.00% Duplicated Code (0.70% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

@mexanick mexanick dismissed kosack’s stale review March 12, 2025 08:47

Changes implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:calib issues related to ctapipe.calib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Additional Muon Parameters in ctapipe for LST Analysis
4 participants