Autostronomy / AstroPhot

A fast, flexible, automated, and differentiable astronomical image 2D forward modelling tool for precise parallel multi-wavelength photometry
https://astrophot.readthedocs.io
GNU General Public License v3.0
87 stars 9 forks source link

Change star nomenclature to point source #126

Closed ConnorStoneAstro closed 7 months ago

ConnorStoneAstro commented 12 months ago

Closes #125 in principle, though more explanation is likely needed. Using the point source nomenclature should make it more clear that these models are intended for any point source, not just stars.

codecov[bot] commented 12 months ago

Codecov Report

Attention: 124 lines in your changes are missing coverage. Please review.

Comparison is base (d44475a) 79.31% compared to head (ef81e43) 79.41%.

Files Patch % Lines
astrophot/image/jacobian_image.py 18.18% 18 Missing :warning:
astrophot/models/psf_model_object.py 87.20% 16 Missing :warning:
astrophot/image/target_image.py 58.33% 15 Missing :warning:
astrophot/image/model_image.py 30.00% 14 Missing :warning:
astrophot/models/point_source.py 81.81% 14 Missing :warning:
astrophot/models/_model_methods.py 87.50% 11 Missing :warning:
astrophot/models/group_psf_model.py 78.78% 7 Missing :warning:
astrophot/image/psf_image.py 83.33% 6 Missing :warning:
astrophot/plots/image.py 78.57% 6 Missing :warning:
astrophot/image/image_header.py 84.37% 5 Missing :warning:
... and 5 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #126 +/- ## ========================================== + Coverage 79.31% 79.41% +0.09% ========================================== Files 87 90 +3 Lines 7252 7665 +413 ========================================== + Hits 5752 6087 +335 - Misses 1500 1578 +78 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ConnorStoneAstro commented 12 months ago

perhaps psf would be better than point as the model type indicator

ConnorStoneAstro commented 11 months ago

I think there's some more work to do here in separating the PSF as (A) the thing that is the convolution kernel from the true image to the realized image on the CCD vs. (B) astrophysical things that appear like point sources in images but aren't quite: Nuker, Sersic profiles.

In AstroPhot there isn't really a strong distinction between convolution kernels and point sources in images. For example here: https://autostronomy.github.io/AstroPhot/AdvancedPSFModels.html#PSF-fitting-with-a-bad-star in cell In [9] you can see the sersic galaxy model is given a different AstroPhot model (Moffat) to use as it's convolution kernel. You can go further and even use a full group model with multiple components as the convolution kernel.

There are some unorthodox PSF models in there like the Sersic and Nuker PSF models, but that is really just to give the user freedom in how they want to construct the PSF. Sersic is just a generalization of a Gaussian which is a typical basic PSF model, and Nuker is effectively a double power law.

So I'm not sure drawing a distinction between the models is the right path, but perhaps I need more explanation on this way of thinking about PSF models?

wmwv commented 11 months ago

So I'm not sure drawing a distinction between the models is the right path, but perhaps I need more explanation on this way of thinking about PSF models?

Hmmm... I see what you're saying mathematically and computationally.

But, conceptually, at least as an observer, I divide these differently in my brain. The PSF is what convolves that scene that hits top of the atmosphere. It convolves everything in the image. The PSF is the same for all objects in an image (with some perhaps smooth spatial variation).

But each object is a different astrophysical object with its own inherent shape.

I think for a new user, in particular, it might be helpful to the user to be pretty clear about the difference between astrophysical model and point source. I fear that blurring the distinction will make it hard for the user to reason about whether or not they're double-convolving.

One particularly use case: I would like to fit a (let's fixed for now) PSF for the image for all objects. I thus want to pass a sersic for the galaxy and delta functions for the stars. These will then get convolved by the PSF. It's hard for me to reason about how to do that in the current way the models and PSFs are done.

In practice, as AstroPhot is used on more data that has been processed through standard astronomical image pipelines, I think it will want to support Gaussians, Moffats, the families of models from PSFEx, and then also the detailed PSFs from spaced-based missions such as: HST, JWST, GALEX, Swift.

ConnorStoneAstro commented 11 months ago

it might be helpful to the user to be pretty clear about the difference between astrophysical model and point source. I fear that blurring the distinction will make it hard for the user to reason about whether or not they're double-convolving.

Ah, I think I see what you are saying. In principle all the subclasses of PSF_Model should actually just be one class called something like Delta_Function (probably actually just Point_Source) which one then convolves with a PSF to get the image of a point source on the sky. In that sense, all the PSF models (moffat, gaussian, pixelated, eventually PSFEx/HST/JWST/...) should be a whole separate category and not Component_Model subclasses at all. Am I understanding your point correctly? In that case I would restructure the models to have the base be AstroPhot_Model which is then subclassed by Component_Model, Group_Model, and PSF_Model, all other models would subclass those three appropriately. Does that seem more intuitive?

Right now AstroPhot avoids double convolving internally by forcing convolution to be off for any PSF_Model object, but the user may not know this and could justifiably be concerned about double convolution.

wmwv commented 11 months ago

In principle all the subclasses of PSF_Model should actually just be one class called something like Delta_Function (probably actually just Point_Source) which one then convolves with a PSF to get the image of a point source on the sky. In that sense, all the PSF models (moffat, gaussian, pixelated, eventually PSFEx/HST/JWST/...) should be a whole separate category and not Component_Model subclasses at all. Am I understanding your point correctly?

Yes, exactly.

Thank you for being open to re-thinking about the design.

ConnorStoneAstro commented 11 months ago

Perfect, I can work on that but it will be a bit slow since I have a number of other things piled up this week.

I'm always happy to re-structure the design if it makes things clearer for users (assuming its feasible computationally). I really want this project to be widely used :)

ConnorStoneAstro commented 11 months ago

So, update on the PSF restructure.

I am building out the new format, but haven't gotten to testing yet. A few things have become clearer along the way.

PSF_Model objects will have a PSF_Image as their target instead of a Target_Image object. This has a few benefits, for one, if you have a stacked empirical PSF then this can be used to initialize the PSF_Model just like how a Component_Model can use a Target_Image to initialize itself. It also handles position information since PSF_Image objects are always centered at (0,0) the PSF_Model objects don't need to define a center, they can assume (0,0) as the center always.

The Point_Source model which is the delta function subclass of the Component_Model now makes more sense in terms of it's meaning than the various star models from before. However, it seems like operationally it is a bit more clunky. To define a point source with a Moffat profile one must now first define a Moffat_PSF model which describes the point spread function, then define a Point_Source model which actually places a delta function somewhere in the image. However, I have made it so that when creating a point source you initialize it with a psf variable which is a PSF_Model object. It would look like:

psf = ap.models.AstroPhot_Model(
    model_type = "moffat psf model",
    target = psf_image,
)
star = ap.models.AstroPhot_Model(
    model_type = "pointsource model",
    target = target_image,
    psf = psf,
)

whereas before only a single model would be defined which is a moffat star model. I have mixed feelings about this since conceptually this is much more clear about what is happening in the code, but operationally it is quite a bit more lines (though the psf would only need to be defined once, so it's less noticeable when fitting many point sources). I'd be interested in your thoughts on this.

One thing that I liked about the original system was that in principle any model in AstroPhot could be used to try an model the PSF. In principle this could mean very complex representations, but perhaps that was more confusing than actually helpful. The new system restricts to only use PSF_model objects which could potentially be limiting (some bad, some good there). I plan to make a variant of Group_Model so that PSF models can be stacked together to make complex representations, but I may end up duplicating a lot of code in order to get the same kind of functionality for both Component_Models and PSF_Models.

Another complication with the Point_Source model is that the core sampling method currently assumes some things that aren't always true anymore. If using a Pixelated_PSF model, the pixels do not really represent a smooth analytic model, in an empirical (pixelated) psf the pixels are already integrated by nature of being directly taken from the image. I think this is pretty solvable just by overloading the sample method. But this among the reasons listed above means this update won't be super quick to finish... Even for non-pixelated models like the moffat, the way they get sampled is pretty different now. Before, to represent a star with a moffat, I would use the position of the star in the image and integrate directly to the pixels. Now, the moffat model is a psf model and so I sample it into a centered PSF_Image which I "convolve" with the delta function by multiplying by the flux, then I do a sub-pixel shift to place the object exactly where it should be. Basically this is the procedure used whenever convolving a PSF with a model, so I already have code for these steps, but it is more computationally expensive than integrating directly to the pixels. I might try and find a work around here just to save compute, not sure yet.

An idea I had for a completely different approach to this was that instead of making a PSF_Model subclass of AstroPhot_Model I could have made it so that all models had a "psf mode" which would be activated any time they were provided a PSF_Image as their target. This would probably be easier to code since it is quite similar to what I already have implemented. And it would allow using any model as a PSF model. However, it breaks the conceptual clarity and would probably be just as, if not more, confusing than the current system. I also couldn't think of a good name for all the "star" models as they are right now since they would kind of pull double duty as face-on galaxy models, and PSF models. So ultimately I think that would just be more confusing.

Ok, I should probably stop here. Let me know what you think so far! I've pushed the state of the code as it is, nothing works but it might help you see what I'm talking about.

ConnorStoneAstro commented 10 months ago

Ok, after puttering away for quite a while, I have the PSF update online! PSF models are now a special class of models that take a PSF_Image as their target instead of a Target_Image. PSF models can be initialized on empirical PSFs just like how regular models can be initialized on target images. A new Point_Source model represents a delta function that gets convolved by a PSF to project into an image. I've also cleaned up a bunch of stuff for how PSF images work and how everything interacts. There is a corresponding branch for the tutorials which shows the updates. Let me know what you think!

ConnorStoneAstro commented 10 months ago

This also closes #124 as the eigen PSF model was added here as well

wmwv commented 7 months ago

@ConnorStoneAstro Is there more work you're planning to do on the branch for this PR before merging?

ConnorStoneAstro commented 7 months ago

@ConnorStoneAstro Is there more work you're planning to do on the branch for this PR before merging?

@wmwv Sorry, no life has just gotten away from me. Lets just pull the trigger now and then everyone can enjoy the new updates :)