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

FWHM #2124

Closed
clara-escanuela opened this issue Nov 18, 2022 · 5 comments · May be fixed by #2151
Closed

FWHM #2124

clara-escanuela opened this issue Nov 18, 2022 · 5 comments · May be fixed by #2151

Comments

@clara-escanuela
Copy link
Contributor

The FWHM of the waveforms could be included in ctapipe to, for example, correct for saturation effects to improve the charge/time resolution in the high charge regime. Not sure where this variable should be stored.

I could implement this myself.

@maxnoe
Copy link
Member

maxnoe commented Nov 18, 2022

Hi @clara-escanuela

I don't think this is an information we really want at the DL1 level, rather something that one specific extractor could calculate if needed?

Some cameras never expect to have saturated pulses (at least not in regime relevant for air showers), because they have a low-gain channel, so always calculating this would add a rather expensive operation, that is in general not needed.

If single-gain cameras need a way to deal with saturated pixels, I think this should be handled in an ImageExtractor internally. It also has implications for the calibration, since you'd need to handle the saturated and non-saturated case differently.

In this regard, discussed also with @FrancaCassol, I think we should change the definition of the calibration coefficient arrays a tiny bit. At the moment it is (n_gains, n_pixels), I'd relax this to: (n_needed_coefficients, n_pixels), so that e.g. LSTCam can continue to use two sets of coefficients for each gain and e.g. flashcam could use one set for unsaturated pulses and one for saturated once.

There are also many different ways you could handle saturation, not just FHWM, e.g. time over the saturation threshold or trying to fit the pulse in the parts that is not saturated.

To make it short: if don't think FWHM is of general use in dl1, we should stick with the two well-defined arrays image and peak_time, but of course FWHM could be useful inside an extractor implementation to deal with saturated pulses.

@FrancaCassol @kosack what do you think?

@kosack
Copy link
Contributor

kosack commented Nov 21, 2022

We had in the past discussed adding more than just the peak-time to at least some image extractors: for example we could compute all pulse-related quantities:

  • peak time
  • rise time
  • fall time
  • width (FWHM)

I think those do not need to be there for all DL1 output, as it adds a lot of overhead, but we could have these as optional computations that are enabled in an option in ImageExtractors. In that case we would just add images to the DL1 container for each of these parameters, leaving them as None by default (so nothing is written to DL1 unless they are filled).

@maxnoe
Copy link
Member

maxnoe commented Nov 21, 2022

Also fine with me!

If any of the reconstructors use these quantities, we need containers for those anyways.

@clara-escanuela
Copy link
Contributor Author

clara-escanuela commented Nov 22, 2022

Sounds good! Thanks. So I will start thinking about implementing these quantities if that's okay

@clara-escanuela clara-escanuela linked a pull request Dec 5, 2022 that will close this issue
@maxnoe
Copy link
Member

maxnoe commented Jan 4, 2023

Turns out we had this issue open already: #1243, closing this one as duplicate, but we still should go forward with this

@maxnoe maxnoe closed this as completed Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants