cta-observatory / ctapipe

Low-level data processing pipeline software for CTAO or similar arrays of Imaging Atmospheric Cherenkov Telescopes
https://ctapipe.readthedocs.org
BSD 3-Clause "New" or "Revised" License
65 stars 269 forks source link

PSF model #2643

Open ctoennis opened 2 weeks ago

ctoennis commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

ctoennis commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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

ctao-dpps-sonarqube[bot] commented 1 week ago

Passed

Analysis Details

0 Issues

Coverage and Duplications

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

ctao-dpps-sonarqube[bot] commented 1 week ago

Passed

Analysis Details

0 Issues

Coverage and Duplications

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

ctao-dpps-sonarqube[bot] commented 1 week ago

Passed

Analysis Details

0 Issues

Coverage and Duplications

Project ID: cta-observatory_ctapipe_AY52EYhuvuGcMFidNyUs

View in SonarQube

orelgueta commented 1 week ago

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.

You are right, it comes out naturally through ray tracing with mirror roughness and misalignment.