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

merge stage1 and reconstruct-muons #1353

Closed
kosack opened this issue May 21, 2020 · 12 comments · Fixed by #2168
Closed

merge stage1 and reconstruct-muons #1353

kosack opened this issue May 21, 2020 · 12 comments · Fixed by #2168

Comments

@kosack
Copy link
Contributor

kosack commented May 21, 2020

There's a lot of overlap between the tools ctapipe-stage1-process and ctapipe-reconstruct-muons. Both output DL1 data, with parameters being one of the options.

I suggest we just drop ctapipe-reconstruct-muons and add an option like --write-muon-parameters=True (similar to --write-parameters) that optionally does the muon detection/reconstruction. This way the same tool can be used for both purposes.

That way, the output files are identical (now they differ), and one can even store both muon parameters and images for testing purposes

--write-images=True --write-parameters=False --write-muon-parameters=True would then produce a DL1 file with simulation info, calibrated images, and muon parameters (can even keep the other parameters enabled as well).

@maxnoe
Copy link
Member

maxnoe commented May 21, 2020

Agreed

@moralejo
Copy link
Contributor

We did a similar thing recently in lstchain. Keep in mind that probably for muon rings the global peak integrator is better: the signal won't have a bias in pixels containing only NSB. So what we did is to re-integrate the waveforms with the same window parameters (width & shift), but using GlobalPeak integrator, whenever an image fulfilled the "muon candidate conditions". With global peak integrator the comparison to MC is more robust against differences in the NSB level.

@moralejo
Copy link
Contributor

moralejo commented Nov 5, 2020

Yes, certainly we will eventually converge with the ctapipe DL1 stage tool, including the standardized muon analysis. But an intermediate "checkpoint" in which we adopt the standard DL1 format, but keep our current muon analysis, will be helpful I think.

@kosack
Copy link
Contributor Author

kosack commented Nov 5, 2020

PR #1499 does a lot towards making this easy. I think we just need to create a new class like the ImageProcessor defined there for muon analysis (MuonProcessor?), and then we call it if that option is enabled. We will need somewhere in the ArrayEvent data structure to put the muon parameters and optical efficiency parameters , so perhaps under event.dl1.tel[x].muon.ring_parameters and event.dl1.tel[x].muon.efficiency_parameters, or we add them to the ImageParameters container (but then have to ignore them when not filled)

So I suggest we define:

class MuonParametersContainer(Container):
     ring_parameters = Field(MuonRingContainer(), ...)
     efficiency_parameters = Field(MuonEfficiencyContainer(), ...)

@maxnoe
Copy link
Member

maxnoe commented Nov 5, 2020

agreed.

@moralejo stressed that the muons should have there own extractor, always using GlobalPeakWindowSum to minimize noise.

And the muon analysis should only run after passing some relatively hard ImageQuery cuts, but I think that's all rather easy with a structure like that.

@kosack
Copy link
Contributor Author

kosack commented Nov 5, 2020

One comment: since it's required in CTA that cameras do a pre-selection of muon candidates, and those are written to a separate file, it may not be necessary to do anything fancy in ctapipe-stage1 to support muons with a different extractor, you would just have a different config file for muon pre-selected files where you would set optoins like CameraCalibrator.extractor_type="GlobalPeakWindowSum", and DL1Writer.write_muons = True, and set the ImageProcessor.ImageQualityQuery to the list of cuts you want to apply.

If current prototypes don't already separate muon-tagged events into a different stream, we could simply make a tool to pre-select them. That way, there is nothing special in ctapipe-stage1 other than the option to enable or disable the muon code.
By that I mean, you would perhaps run ctapipe-stage1 two times: first to generate calibrated images, then use a tool to loop over the DL1 image file and extract muon-like events, then run ctapipe-stage1 again on the DL1 file to generate the parameters (with the DL1EventSource, this should be possible)

@maxnoe
Copy link
Member

maxnoe commented Nov 5, 2020

I think for the moment it would be nice to enable this for the prototypes that are not yet doing it. @moralejo was very adamant about not having to run the tool twice over the large LST data.

@kosack
Copy link
Contributor Author

kosack commented Nov 5, 2020

I think for the moment it would be nice to enable this for the prototypes that are not yet doing it. @moralejo was very adamant about not having to run the tool twice over the large LST data.

It wouldn't really be running it twice - the first time you would do the image step, and the second time the parameters starting from the images, so the total CPU time would be the same (other than a bit of extra IO). I'd rather not have a ultra-complex analysis script that will be harder to maintain. It also complicates the output since it means the muon parameters would be computed with different images than the other parameters, but we only store one set of images. I think that makes things very inconsistent and ugly.

The workflow should be:

  • regular_events → events.dl0 → stage1 (standard config) → standard dl1
  • possible_muon_events → muons.dl0 → stage1 (muon config) → muon dl1 → overall optical efficiency

If both regular and muon events could use the same integrator, then you could do both at once, but if not I think it has to be split

@kosack
Copy link
Contributor Author

kosack commented Nov 5, 2020

It we have to (and I still think this is ugly and over-complicated to support a missing but required feature of the telescope), we would have to construct two CameraCalibrators one for muons and one for normal events. Then we run the Muon CameraCalibrator, detect if it's a muon event by doing a muon parameterization, write the event type somewhere (muon or not muon), if not remove the DL1 images, re-process the event with the "normal" calibrator, and then parameterize. That will be a lot of branching logic, but would probably work, even though again the images in the output would depend on the event type.

I'd prefer something that complex be a prototype-specific tool, and just keep the standard tools simple. In the future, LST DL0 data will (must) include a trigger_type that tells you if it's a possible muon or not, so that can be used to avoid duplicating the calibrator and image processing.

@moralejo
Copy link
Contributor

moralejo commented Nov 5, 2020

One comment: since it's required in CTA that cameras do a pre-selection of muon candidates, and those are written to a separate file,

This means they also have to be written to the 'normal' DL0 file, right? The filter won't be perfect, and it is also convenient to have all recorded events in the same stream (e.g. for dead time calculations). Also, the muon ring will sometimes be part of a stereo event (in case of LST, always).

@moralejo
Copy link
Contributor

moralejo commented Nov 5, 2020

I'd prefer something that complex be a prototype-specific tool, and just keep the standard tools simple. In the future, LST DL0 data will (must) include a trigger_type that tells you if it's a possible muon or not, so that can be used to avoid duplicating the calibrator and image processing.

As far as I know, currently we cannot write out from lstchain raw data in the same format in which the zfits writers store them. So I do not think this is a realistic short-term solution. It seems to me that before moving to use the DL1 stage tool, we should make an intermediate step in which we just write out LST1's DL1 output in the standard ctapipe format (but keep the muon analysis the way we do it now, embedded in the DL1 production). Of course, the moving of low-level calibrations to ctapipe_io_lst can still be done straightaway in such a scheme.

@RuneDominik
Copy link
Member

With #2168 merged this can be closed.

@maxnoe maxnoe linked a pull request Jan 27, 2023 that will close this issue
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 a pull request may close this issue.

4 participants