PSF model #2643

ctoennis · 2024-11-10T12:35:18Z

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 · 2024-11-11T14:02:26Z

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 · 2024-11-11T14:03:24Z

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

ctoennis · 2024-11-11T14:08:00Z

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 · 2024-11-11T14:08:39Z

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 · 2024-11-11T14:59:40Z

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 · 2024-11-11T15:03:29Z

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 · 2024-11-11T15:06:29Z

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

kosack · 2024-11-11T15:17:59Z

src/ctapipe/instrument/optics.py

+        """
+
+    @classmethod
+    def from_name(cls, name, **kwargs):


could be confusing here... In the instrument model, we use from_name to load an instance of named model from a file (e.g. Telecope.from_name("MST_MST_NectarCam")), not to get a sub-class... this is a general API conflict with the Component classes, however, which is a larger issue, but if this is in instrument, it should at least be consistent.

I have renamed it for now to avoid confusion. The main purpose of this feature was meant to allow the processing of star images to be more easily configurable. The class that processes strar images (e.g. find the stars) can be configured then with a string giving the name of the PSFModel subclass to use. If there is a better way to configure that tool later i could then remove this.

kosack · 2024-11-11T15:30:37Z

src/ctapipe/instrument/optics.py

+        return requested_subclass(**kwargs)
+
+    @classmethod
+    def non_abstract_subclasses(cls):


not needed I think. This is not a Component class.

i am removing it

ctoennis · 2024-11-11T15:34:43Z

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 · 2024-11-12T13:00:32Z

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

maxnoe · 2024-11-13T15:16:33Z

src/ctapipe/instrument/optics.py

+        Parameters
+        ----------
+        asymmetry_params: list
+            Parameters describing the dependency of the asymmetry of the psf on the distance to the center of the camera


These parameters need better explanations and possibly units.

I suggest to add the relevant formulas to the docstring, see

https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-math

I'll add units where possible, but for the most part they won't have them. I will add the documentation when I arrive home.

this is an ansatz recursive parametrization (parametrization of the parameters) :)
Done empirically by fitting the best-fit values of the PSF parameters for simulations of stars with different offset. The formulas should indeed be provided as a part of the documentation.

src/ctapipe/instrument/optics.py

maxnoe · 2024-11-13T15:18:22Z

src/ctapipe/instrument/optics.py

+        self.radial_pdf_params = (k, self.radial_pdf_params[1], sr)
+        self.azimuthal_pdf_params = (self.azimuthal_pdf_params[0], sf)
+
+    def update_location(self, r, f):


Why do we need an "update" function? Do we have to pre-compute expensive operations?

Why is evaluating the PSF not enough?

I have right now 2 use cases for this PSF model: processing variance images (e.g. finding stars in variance images) and fitting the PSF parameters based on stars in variance images. for the former i need to evaluate it the PSF at different locations for each possible star location, in the latter i also need to vary the psf parameters for fitting. It made sense to me to have these 2 functions available to modify these parameters.

Also just to clarify, update location is where the "point" in point spread function is, as in if you want the PSF for a point source at r = 1m and f = 1.0rad these coordinates will need to be put in this function.

i will try and make the documentation more clear.

The question is why this point needs to be state on the class and cannot just be evaluated.

I believe this is a remnant of the debugging code I used during the development... :)
@ctoennis, we don't need this step to be factored out like this anymore.

ok, i'll put it all together then. I am having a little issue with the docustrings i'm sorting out now. I'll get to it after that.

src/ctapipe/instrument/optics.py

maxnoe · 2024-11-13T15:19:46Z

src/ctapipe/instrument/optics.py

+        az_scale_params=[0.24271557, 7.5511501, 0.02037972],
+    ):
+        """
+        PSF model, describing purely the effect of coma aberration on the PSF


Is there a citable reference for the math here?

Here it is: https://www.aanda.org/articles/aa/full_html/2023/11/aa47128-23/aa47128-23.html
I will add this to the documentation

The bib file is here:
https://github.com/cta-observatory/ctapipe/blob/main/docs/references.bib

you can then cite using:

:cite:p:`<bibtex key>`

awesome. will do

Which makes me realize we didn't add a reference for the hipparcos catalog

i can add that.

maxnoe · 2024-11-13T15:39:26Z

src/ctapipe/instrument/optics.py

+
+    def __init__(
+        self,
+        asymmetry_params=[0.49244797, 9.23573115, 0.15216096],


Where do these magic numbers come from?

How are you expecting we determine these numbers? Is there a tool to compute them from a series of PSF images or pedestal images?

How do you expect them to be fed into ctapipe for the telescopes?

I got these numbers from https://gitlab.cern.ch/cta-unige/startracker (line [162 https://gitlab.cern.ch/cta-unige/startracker/-/blob/master/startracker/analysis/image.py?ref_type=heads]). @mexanick generated these parameters based on some fits. I can remove these as default.

So, when i will use this for extracting stars these parameters will not need to be altered. However for fitting the PSF using stars they will need to be altered like a regular variable. My idea would be to have the tool that extracts stars to have these parameters as traitlets, so that when you process data they can just be in a yml file.

For PSF fitting we don't need these exact values. I will remove these values here and have them in the config files later.

So, when i will use this for extracting stars these parameters will not need to be altered

for LST-1, right?

Ah, yes, they need to be configured per telescope.

Ok, then I think we should discuss a bit about API.

I think, it's natural then to make this a TelescopeComponent and if there is code that can take a list of images and fit the result, we could have that as a

@classmethod def fit(self, ...) -> Parameters:

here

that makes sense. For the fitting though my idea was to fit the PSF based on images with multiple stars. I have made some draft of a function that would generate a picture based on star locations, magnitudes and psf parameters that would be used as a model for fitting:

def get_star_pdf(r0, f0, geometry, psf, n_pdf_bins, pdf_bin_size, focal_length): image = np.zeros(len(geometry)) dr = pdf_bin_size * np.rad2deg(np.arctan(1 / focal_length.to_value(u.m))) / 3600.0 r = np.linspace( r0 - dr * n_pdf_bins / 2.0, r0 + dr * n_pdf_bins / 2.0, n_pdf_bins, ) df = np.deg2rad(pdf_bin_size / 3600.0) * 100 f = np.linspace( f0 - df * n_pdf_bins / 2.0, f0 + df * n_pdf_bins / 2.0, n_pdf_bins, ) for r_ in r: for f_ in f: val = psf.pdf(r_, f_, r0, f0) * dr * df x, y, _ = r_ * s2c(f_, 0) pixelN = geometry.position_to_pix_index(x * u.m, y * u.m) if pixelN > -1 and pixelN < len(image): image[pixelN] += val return image def StarImageGenerator( radius, phi, magnitude, n_pdf_bins, pdf_bin_size, psf_model, camera_geometry, focal_length, ): """ :param list stars: list of star containers, stars to be placed in image :param dict psf_model_pars: psf model parameters """ image = np.zeros(len(camera_geometry)) for r, p, m in zip(radius, phi, magnitude): image += np.power(10,-0.4*m) * get_star_pdf( r, p, camera_geometry, psf_model, n_pdf_bins, pdf_bin_size, focal_length ) return image

I made the PSFModel into a TelescopeComponent. I will need to see how i will handle the fitting in detail, but i can work that out.

maxnoe · 2024-11-14T16:53:35Z

src/ctapipe/instrument/optics.py

+
+
+class PSFModel(TelescopeComponent):
+    def __init__(self, subarray, **kwargs):


not needed, since identical with baseclass.

Docstring should be at the class definition, not __init__.

OK, I'll fix it.

maxnoe · 2024-11-14T17:16:24Z

src/ctapipe/instrument/optics.py

+        super().__init__(subarray=subarray, **kwargs)
+
+    @abstractmethod
+    def pdf(self, *args):


This needs a docstring so that subclass implementers know what is expected.

Also the signature should be the same for all classes, so it should be explicit here in the interface definition.

Christoph Toennis added 3 commits October 31, 2024 13:02

adding PSF model

4aeaa05

adding tests and documentation

295dd24

adding changelog, fixing test

d171084

ctoennis added enhancement calibration new functionality labels Nov 10, 2024

ctoennis self-assigned this Nov 10, 2024

renaming changelog

ae2652b

ctoennis requested review from maxnoe and kosack November 10, 2024 13:01

Moving the psf models to ctapipe.instrument.optics

0bcd117

Updating docustrings

038376d

kosack reviewed Nov 11, 2024

View reviewed changes

upate docustring

c25c5b0

Updating and renaming from_name method

8376ab0

ctoennis mentioned this pull request Nov 12, 2024

Add psf model to InstrumentDescription #2647

Open

ctoennis requested a review from kosack November 12, 2024 13:05

maxnoe reviewed Nov 13, 2024

View reviewed changes

maxnoe requested changes Nov 13, 2024

View reviewed changes

maxnoe reviewed Nov 13, 2024

View reviewed changes

Christoph Toennis added 7 commits November 13, 2024 20:49

updating doumentation with comamodel math

552d5d7

Fixing issues in documentation

01a98ac

update to docustrings

bb13838

fixing issue in docustring

e912799

removing the update_location method

962e103

fixing more issues with docustrings

ba865dc

Refactoring as TelescopeComponent

ac7140d

maxnoe reviewed Nov 14, 2024

View reviewed changes



		class PSFModel(TelescopeComponent):
		def __init__(self, subarray, **kwargs):

PSF model #2643

Are you sure you want to change the base?

PSF model #2643

Conversation

ctoennis commented Nov 10, 2024

kosack commented Nov 11, 2024

maxnoe commented Nov 11, 2024

ctoennis commented Nov 11, 2024

ctoennis commented Nov 11, 2024

kosack commented Nov 11, 2024

maxnoe commented Nov 11, 2024

kosack commented Nov 11, 2024 • edited Loading

kosack Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctoennis commented Nov 11, 2024

ctoennis commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctoennis Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kosack commented Nov 11, 2024 •

edited

Loading

kosack Nov 11, 2024 •

edited

Loading

ctoennis Nov 14, 2024 •

edited

Loading