PSF model

[ctoennis]

I am adding PSF models to ctapipe.image.psf_model. I made a parent class called PSFModel and a PSF model based on pure coma abberation called ComaModel.

This will be used in the pointing calculation using star tracking and PSF fitting using stars.

[kosack]

Quick question: are these models applicable for both gamma-ray and optical PSF? If not, maybe call it OpticalPSFModel to be more clear where it should be used.

[maxnoe]

I think this is purely optical, not IRF. Also, I think the appropriate place would be in ctapipe.instrument.optics

[ctoennis]

Quick question: are these models applicable for both gamma-ray and optical PSF? If not, maybe call it OpticalPSFModel to be more clear where it should be used.

The model accounts for coma abbarations and the default parameters i have would be for photons in the optical range. With different parameters this should work with gamma photons, but i am not sure. I could call the ComeModel OpticalComaModel and keep the parent class as is. Then later models specifically for gamma photons can be added.

[ctoennis]

I think this is purely optical, not IRF. Also, I think the appropriate place would be in ctapipe.instrument.optics

Makes sense. I will move it to optics.

[kosack]

Side note: we should check with what is used in SimPipe's instrument model, to ensure what is output here is compatible with what the simulations expect.

[maxnoe]

Side note: we should check with what is used in SimPipe's instrument model, to ensure what is output here is compatible with what the simulations expect.

I don't think there is a model like this in SimPipe, but @orelgueta or @GernotMaier can confirm. Rather coma is naturally resulting from the definition of the mirror facets.

[kosack]

I think the appropriate place would be in ctapipe.instrument.optics

The instrument module is where we define the InstrumentDescription model (which currently is collected under SubarrayDescription class here in ctapipe). Would be useful then to integrate it with the rest of the model, i.e. add a place for it in OpticsDescription, and add serialization so that it can be written and read. A collection of these could be serialized to a new table (one row per telescope), as we do with other OpticsDescription parts (SubarrayDescription.to_table(kind="optical_psf"), or else just in OpticsDescription... ) . That could be in another PR if you like, but in that case we should open an issue to make sure this new part of the model is properly integrated

Edit: to be more clear, I would expect something like this to work:

psf : Optional[PSFModel]  = subarray.tel[tel_id].optics.psf

if psf: 
     plot_psf(psf)  # some general function that plots any PSFModel

[ctoennis]

I think the appropriate place would be in ctapipe.instrument.optics

The instrument module is where we define the InstrumentDescription model (which currently is collected under SubarrayDescription class here in ctapipe). Would be useful then to integrate it with the rest of the model, i.e. add a place for it in OpticsDescription, and add serialization so that it can be written and read. A collection of these could be serialized to a new table (one row per telescope), as we do with other OpticsDescription parts (SubarrayDescription.to_table(kind="optical_psf"), or else just in OpticsDescription... ) . That could be in another PR if you like, but in that case we should open an issue to make sure this new part of the model is properly integrated

Edit: to be more clear, I would expect something like this to work:

psf : Optional[PSFModel]  = subarray.tel[tel_id].optics.psf

if psf: 
     plot_psf(psf)  # some general function that plots any PSFModel

if it is okay then i will open an issue for this

[ctoennis]

I think the appropriate place would be in ctapipe.instrument.optics

The instrument module is where we define the InstrumentDescription model (which currently is collected under SubarrayDescription class here in ctapipe). Would be useful then to integrate it with the rest of the model, i.e. add a place for it in OpticsDescription, and add serialization so that it can be written and read. A collection of these could be serialized to a new table (one row per telescope), as we do with other OpticsDescription parts (SubarrayDescription.to_table(kind="optical_psf"), or else just in OpticsDescription... ) . That could be in another PR if you like, but in that case we should open an issue to make sure this new part of the model is properly integrated
Edit: to be more clear, I would expect something like this to work:

psf : Optional[PSFModel]  = subarray.tel[tel_id].optics.psf

if psf: 
     plot_psf(psf)  # some general function that plots any PSFModel

if it is okay then i will open an issue for this

Here it is: #2647

[mexanick]

@maxnoe , continuing the discussion started in #2689 :

The parametrization in polar coordinates works well under assumption that the radial and polar components are independent, i.e. $\vec{r}\perp\vec{\phi}$. This breaks at low radii, that's why some workarounds were introduced. However, with the Cherenkov camera we are not that much interested in the stars images that close to the center of the camera (we need a lever arm and a larger light spread to obtain reasonable precision), that's why no particular attention was given to this kind of edge cases before. I agree that it looks a bit ugly, and a better parametrization can be considered, but I would propose to move on for the moment with this and add other parameterizations in the future as needed (there'll be need to do this anyway for double-mirror telescopes).

[maxnoe]

When evaluating at 0,0 I got a warning:

/home/maxnoe/Uni/CTA/ctapipe/src/ctapipe/instrument/optics.py:430: RuntimeWarning: invalid value encountered in scalar divide
  dphi = np.arcsin(chord_length / (2 * r0))

otherwise looks good

[maxnoe]

The math does not render properly: https://ctapipe--2643.org.readthedocs.build/en/2643/api/ctapipe.instrument.ComaPSFModel.html#ctapipe.instrument.ComaPSFModel

[mexanick]

The math does not render properly: https://ctapipe--2643.org.readthedocs.build/en/2643/api/ctapipe.instrument.ComaPSFModel.html#ctapipe.instrument.ComaPSFModel

this is fixed now. The only thing, is that there's an autosummary for the attributes, so the documentation for them is basically repeated twice. Also, the init docstring is ignored. Any suggestion on what to do with this? Shall I remove the Attributes summary from the class-level docstring and add there what's in the init's one?

[maxnoe]

Also, the init docstring is ignored

Yes, documentation should be in the class docstring, not __init__. Add the Parameters section to the class docstring

[mexanick]

@kosack can you please take a look? I shouldn't participate in the review any further as I've contributed to the code.

[maxnoe]

@mexanick Could you please fix the sonarqube complaints:

https://sonar-cta-dpps.zeuthen.desy.de/project/issues?pullRequest=2643&resolved=false&types=CODE_SMELL&inNewCodePeriod=true&id=cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

[ctao-dpps-sonarqube]

Analysis Details

0 Issues

• 0 Bugs
• 0 Vulnerabilities
• 0 Code Smells

Coverage and Duplications

• 98.80% Coverage (94.20% Estimated after merge)
• 0.00% Duplicated Code (0.70% Estimated after merge)

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube