Repository navigation
Modeling-related changes to PSF/PRF photometry #48
Description
Comments from @eteq, copied from #26:
- First, a few comments about how
astropy.modelingis used here. It looks to me likeDiscretePRFandGaussianPSFare the only allowed classes, rather than arbitrary models fromastropy.modeling.models. This seems to mostly stem from the use of thefitmethod of the PSF/PRF classes. Why is this necessary? I can certainly imagine wanting to try other PSF models (most crucially, the Moffat function). So I think it would be better ifDiscretePRFandGaussianPSFobeyed standard convnetions forFittable2DModels, and the work of massaging those models into a form appropriate forpsf_photometryshould be done elsewhere (either a container class or in thepsf_photometryfunction itself. Otherwise, we effectively have two slightly different model-fitting frameworks - Why doesn't
GaussianPSFusemodeling.models.Gaussian2D? It seems to have implemented all the gaussian behavior separately? - Why is the
fitterhard-coded into the PSF/PRF class instead of following the convention used inastro.modelingof having it be a separate object? Don't we want to allow other fitting methods? - I think it would make sense to try to make
psf_photometryandaperture_photometrymore consistent. E.g., right nowaperture_photometrygets its centers from 2 argumentsxcandyx, whilepsf_photometryaccepts a singlepositionsargument. At the very least, these should be the same. It also might be useful to allow usingaperture_photometryapertures to define the region in which a star is fit (although maybe that's more of a feature request).
Activity
[-]Fixes to PSF/PRF photometry[/-][+]Modeling-related fixes to PSF/PRF photometry[/+][-]Modeling-related fixes to PSF/PRF photometry[/-][+]Modeling-related changes to PSF/PRF photometry[/+]adonath commentedon May 21, 2014
I'd like to make a few comments on the mentioned issues:
The fit method should be definitely seperated from the models. If I remember correctly, this is a relict from the old
astropy.modelingAPI, where the fitter had to be instantced with the model.Once the
psf.fitmethod is removed from the model it should be fairly easy to makepsf_photometrywork with arbitrary models.The only, but very important, difference is that the evaluation of the
GaussianPSFis implemented using the Error function. This is equivalent to integrating the model over the pixel, which avoids a bias of the size and flux for very small sources (~ 1 pixel). One drawback is, that it only works in pixel coordinates so far. It could make sense to move this to 'astropy.modeling' and introduce aGaussian2DIntmodel or even have an integrate method for theGaussian2Dmodel? I remember that similar suggestions were made in the past. What's the status of it?cc @eteq
larrybradley commentedon May 21, 2014
This is a more general comment, but I think it would be useful to add methods to the
astropymodels based on the current options indiscretize_model()from theconvolutionsubpackage, i.e. move the discretize methods to themodelingsubpackage.eteq commentedon May 26, 2014
@adonath - Ah, I didn't appreciate the difference that the
photutilsversion integrates over the pixels. I agree with you that it makes sense to add anintegratemethod that could be overridden for particular models to do fast analytic integrals to the coremodelingpackage.That definitely won't make it for v0.4, though... so two ways to move forward with
photutilsnow:SubsampledGaussianPSForGaussianIntPSFor something like that to make the distinction clearer.Gaussian2Din astropy, but add anintegrate_prfmethod that's customized for that particular class. Then if integral functionality makes it into astropy 1.0,photutilscan chagne the implementation forintegrate_prfto use that as needed.I also agree with @larrybradley that
discretize_modelwould be nice as a method. We welcome PRs! 😉cdeil commentedon Jun 7, 2014
Just so we don't forget, @larrybradley pointed out some concrete suggestions for improvement of
GaussianPSFhere: #54 (comment)hamogu commentedon Feb 5, 2016
Reply to @larrybradley (a few comment above at #48 (comment) ): opened astropy/astropy#4572 for that.
Changes to Gaussian PSF
Existing tests for PSF/PRF now pass.
Changes to Gaussian PSF
Existing tests for PSF/PRF now pass.