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 analysis r0 to dl1 specific function #556

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

Conversation

vuillaut
Copy link
Member

@vuillaut vuillaut commented Nov 5, 2020

step by step cleaning of the r0_to_dl1 step
In preparation for stage1: the final goal is to use directly the stage1 but a step by step approach and some code cleaning will make things easier I think and help us understand better the current status.

@codecov
Copy link

codecov bot commented Nov 5, 2020

Codecov Report

Merging #556 (6361784) into master (ccbb35d) will decrease coverage by 0.03%.
The diff coverage is 3.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #556      +/-   ##
==========================================
- Coverage   37.46%   37.43%   -0.04%     
==========================================
  Files          82       82              
  Lines        7350     7352       +2     
==========================================
- Hits         2754     2752       -2     
- Misses       4596     4600       +4     
Impacted Files Coverage Δ
lstchain/reco/r0_to_dl1.py 61.41% <3.70%> (-1.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccbb35d...6361784. Read the comment docs.

moralejo
moralejo previously approved these changes Nov 5, 2020
Copy link
Collaborator

@moralejo moralejo left a comment

Choose a reason for hiding this comment

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

Look ok to me, thanks for this clean-up.
You say "in preparation of stage 1", but if we move to using directly the stage 1 tool (with all low-level calibs in ctapipe_io_lst), the whole muon analysis will have to be inside ctapipe, I think. On the other hand, that will not be the case if we just introduce a DL1 writer "à la ctapipe" in lstchain's r0_to_dl1, which is perhaps a faster path to have ctapipe-compatible LST1 DL1 files.

@maxnoe
Copy link
Member

maxnoe commented Nov 5, 2020

@moralejo I have not yet come around to doing it, because it is not needed until the rest works, but see cta-observatory/ctapipe#1353

@rlopezcoto
Copy link
Contributor

Hi, thanks @vuillaut
did you check that this works and does not modify any result in the muon analysis?
Unfortunately I think there are not many tests written on this part (partially because of the lack of r0 data to test it against), which we should correct in the future, but it would be great that this is checked to avoid future problems with modified results

@vuillaut
Copy link
Member Author

vuillaut commented Nov 5, 2020

Hi, thanks @vuillaut
did you check that this works and does not modify any result in the muon analysis?
Unfortunately I think there are not many tests written on this part (partially because of the lack of r0 data to test it against), which we should correct in the future, but it would be great that this is checked to avoid future problems with modified results

Yes, it works, I get the same muon file after processing a real data file.
Couldn't agree more about unit tests.

# in calculation of muon ring time (peak sample):
min_pe_for_muon_t_calc = 10.

# FIXME: no need to read telescope characteristics like foclen for every event!
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can remove this FIXME comment, it is a pretty old comment in which I misinterpreted that this would imply some re-reading from disk.


bad_pixels = event.mon.tel[telescope_id].calibration.unusable_pixels[0]
# Set to 0 unreliable pixels:
image = event.dl1.tel[telescope_id].image*(~bad_pixels)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there in ctapipe already some pixel interpolator? Would be interesting to test it. Should be better in case of isolated bad pixels (perhaps a bit dangerous when a whole pixel cluster is unreliable).

Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is, but yes, I think it is necessary.

Copy link
Member

Choose a reason for hiding this comment

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

It should be easy enough to implement approaches like a mean of correct neighbors using the neighbor matrix.

@maxnoe
Copy link
Member

maxnoe commented Dec 13, 2021

Can we update this? I still think it would be good to separate the large imperative code in r0_to_dl1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants