Repository: astropy/photutils

Issue:
About PSF model
Hi,
When I got the PSF model from EPSFBuilder. Could the model be used as the parameter in IterativelySubtractedPSFPhotometry: psf_model? It raise the error lile:
File "/home/yf/.conda/envs/python3/lib/python3.6/site-packages/photutils/aperture/circle.py", line 124, in __init__
    if r < 0:
TypeError: '<' not supported between instances of 'NoneType' and 'int'
Is that mean the EPSFBuilder does not return a proper 2D PSF model, or I made mistakes somewhere. 

Discussion:
Onoddil: This PR also fixes the issue where ``IterativelySubtractedPSFPhotometry`` passed ``bkg_estimator=None`` does not automatically set ``self._residual_image`` at an initial pass, and thus finder callables will fail with ``self._residual_image=None`` as the image to find sources in.
Onoddil: Changes have been made as requested @eteq - please review when possible.
eteq: #903 somewhat collided with this as it turns out, but this provides a somewhat better solution for some cases.  So in an out-of-band discussion, @Onoddil, @larrybradley, and I think the way to move forward is to adjust it so that this PR's solution is adopted for models where `fitshape` exists, but for those where it does *not*, keep the behavior of #903.  That maintains the "helpful error message" aspect of #903, while also giving a reasonable fallback (with warning) for cases like epsf or fittableimagemodel where there's a reasonable guess available.
eteq: Oops that last comment was slightly off - the solution I was proposing was for PSF models that are ePSF models (or possibly all `FittableImageModels`?), not just those with a `fitshape` (since the `fitshape` is set by the photometry object not the model)
Onoddil: I have updated this PR to rebase onto current master and update based on previous discussions. While the previous PR #903 is an acceptable fallback condition, I do still prefer a less harsh fallback, so am in favour of this PR overriding the previous fallback condition.

While returning an error to the user to instruct them to add the parameter I think that the `fitshape` fallback is, for most users, a valid default. I have, however, updated the warning raised while doing so to point out to the user that they may want to consider if their `fitshape` _is_ appropriate, and to perhaps supply an `aperture_radius` anyway. This is fine because the `aperture_radius` is only used to calculate `flux_0`, an initial, order-of-magnitude starting flux to be passed to the main fitter, and so it doesn't have to be particularly precise.

The PR is currently blocked by #942 as I split out the very minor part of the original PR which was unrelated to this code change but is necessary for the `IterativelySubtractedPSFPhotometry` test in the tests to pass, so once that is merged I will rebase again and then the CI can be checked (I have verified it on my machine with the two lines back in, but you can't be too careful of course). I pushed these changes now so that they can be checked and discussed sooner rather than later.

I also wasn't 100% sure if we had done the 0.7 release as of yet so I've updated the changelog as though we haven't, since `CHANGES.rst` doesn't have a 0.8 changelog, but let me know if that is likely to change before the PR will be merged and I'll update it again.

cc @eteq @larrybradley for discussion of whether we still need the final `ValueError` or if the order of magnitude initial guess of `fitshape`'s aperture is good enough for most cases.
larrybradley: @Onoddil Since we already allow `aperture_radius` to be set by model FWHM, this is fine if `fitshape` can be determined.
Onoddil: I have pulled out the fallback aperture radius check into a helper function in `BasicPSFPhotometry` and applied it to both that and `IterativelySubtractedPSFPhotometry`. This should make more obvious the intent to set `aperture_radius` and let the next codeblock handle what happens if `init_guesses` isn't supplied, or is supplied without `flux_0` in `BasicPSFPhotometry.do_photometry`.

Note: this PR currently fails because I accidentally uncovered a bug in `IterativelySubtractedPSFPhotometry`, so it will require a rebase onto a subsequent PR to ensure the test passes before it can also be accepted.
Onoddil: I have rebased this branch on to #986 and it now passes.

Waiting for review again, having resolved the above issues (hopefully!) - cc @eteq @larrybradley 

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 4fcdb2de..a2bd14aa 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -84,6 +84,9 @@ Bug Fixes
   - Fixed a bug that caused photometry to fail on an ``EPSFmodel`` with
     multiple stars in a group. [#1135]
 
+  - Added a fallback ``aperture_radius`` for PSF models without a FWHM
+    or sigma attribute, raising a warning. [#740]
+
 - ``photutils.segmentation``
 
   - Fixed ``SourceProperties`` ``local_background`` to work with
diff --git a/photutils/psf/photometry.py b/photutils/psf/photometry.py
index da313a72..7b757f8d 100644
--- a/photutils/psf/photometry.py
+++ b/photutils/psf/photometry.py
@@ -192,6 +192,35 @@ class BasicPSFPhotometry:
 
         return self._residual_image
 
+    def set_aperture_radius(self):
+        """
+        Sets the fallback aperture radius for initial flux calculations
+        in cases where no flux is supplied for a given star.
+        """
+
+        if hasattr(self.psf_model, 'fwhm'):
+            self.aperture_radius = self.psf_model.fwhm.value
+        elif hasattr(self.psf_model, 'sigma'):
+            self.aperture_radius = (self.psf_model.sigma.value *
+                                    gaussian_sigma_to_fwhm)
+        # If PSF model doesn't have FWHM or sigma value -- as it
+        # is not a Gaussian; most likely because it's an ePSF --
+        # then we fall back on fitting a circle of the average
+        # size of the fitting box. As ``fitshape`` is the width
+        # of the box, we need (width-1)/2 as the radius.
+        else:
+            self.aperture_radius = float(np.amin((np.asanyarray(
+                                         self.fitshape) - 1) / 2))
+            warnings.warn('aperture_radius is None and could not '
+                          'be determined by psf_model. Setting '
+                          'radius to the smallest fitshape size. '
+                          'This aperture radius will be used if '
+                          'initial fluxes require computing for any '
+                          'input stars. If fitshape is significantly '
+                          'larger than the psf_model core lengthscale, '
+                          'consider supplying a specific aperture_radius.',
+                          AstropyUserWarning)
+
     def __call__(self, image, init_guesses=None):
         """
         Performs PSF photometry. See `do_photometry` for more details
@@ -251,35 +280,7 @@ class BasicPSFPhotometry:
             image = image - self.bkg_estimator(image)
 
         if self.aperture_radius is None:
-            if hasattr(self.psf_model, 'fwhm'):
-                self.aperture_radius = self.psf_model.fwhm.value
-            elif hasattr(self.psf_model, 'sigma'):
-                self.aperture_radius = (self.psf_model.sigma.value *
-                                        gaussian_sigma_to_fwhm)
-
-        if self.aperture_radius is None:
-            if init_guesses is None:
-                raise ValueError('aperture_radius was not input and could '
-                                 'not be determined by the input psf_model '
-                                 '(e.g., an EPSFModel).  For tabular PSF '
-                                 'models, you must input the aperture_radius '
-                                 'keyword.  For analytical PSF models, you '
-                                 'must either input the aperture_radius '
-                                 'keyword or define a fwhm or sigma '
-                                 'attribute on your input psf_model.')
-
-            if (init_guesses is not None and
-                    'flux_0' not in init_guesses.colnames):
-                raise ValueError('init_guesses were input, but the "flux_0" '
-                                 'column was not present.  Initial fluxes '
-                                 'cannot be calculated because '
-                                 'aperture_radius must was not input and '
-                                 'could not be determined by the input '
-                                 'psf_model (e.g., an EPSFModel).  For '
-                                 'analytical PSF models, you must either '
-                                 'input the aperture_radius keyword or '
-                                 'define a fwhm or sigma attribute on your '
-                                 'input psf_model.')
+            self.set_aperture_radius()
 
         if init_guesses is not None:
             # make sure the code does not modify user's input
@@ -736,11 +737,7 @@ class IterativelySubtractedPSFPhotometry(BasicPSFPhotometry):
                 self._residual_image = image
 
             if self.aperture_radius is None:
-                if hasattr(self.psf_model, 'fwhm'):
-                    self.aperture_radius = self.psf_model.fwhm.value
-                elif hasattr(self.psf_model, 'sigma'):
-                    self.aperture_radius = (self.psf_model.sigma.value *
-                                            gaussian_sigma_to_fwhm)
+                self.set_aperture_radius()
 
             output_table = self._do_photometry()
         return output_table


Test patch:
diff --git a/photutils/psf/tests/test_photometry.py b/photutils/psf/tests/test_photometry.py
index 06d6de01..8df27fcd 100644
--- a/photutils/psf/tests/test_photometry.py
+++ b/photutils/psf/tests/test_photometry.py
@@ -485,25 +485,82 @@ def test_psf_boundary():
 
 
 @pytest.mark.skipif('not HAS_SCIPY')
-def test_aperture_radius_value_error():
+def test_default_aperture_radius():
     """
-    Test that a ValueError is raised for tabular PSF models when
-    aperture_radius is not defined.
+    Test psf_photometry with non-Gaussian model, such that it raises a
+    warning about aperture_radius.
     """
+    def tophatfinder(image):
+        """ Simple top hat finder function for use with a top hat PRF"""
+        fluxes = np.unique(image[image > 1])
+        table = Table(names=['id', 'xcentroid', 'ycentroid', 'flux'],
+                      dtype=[int, float, float, float])
+        for n, f in enumerate(fluxes):
+            ys, xs = np.where(image == f)
+            x = np.mean(xs)
+            y = np.mean(ys)
+            table.add_row([int(n+1), x, y, f*9])
+        table.sort(['flux'])
 
-    prf = DiscretePRF(test_psf, subsampling=1)
+        return table
+
+    prf = np.zeros((7, 7), float)
+    prf[2:5, 2:5] = 1/9
+    prf = FittableImageModel(prf)
+
+    img = np.zeros((50, 50), float)
+    x0 = [38, 20, 35]
+    y0 = [20, 5, 40]
+    f0 = [50, 100, 200]
+    for x, y, f in zip(x0, y0, f0):
+        img[y-1:y+2, x-1:x+2] = f/9
 
+    intab = Table(data=[[37, 19.6, 34.9], [19.6, 4.5, 40.1]],
+                  names=['x_0', 'y_0'])
+
+    basic_phot = BasicPSFPhotometry(group_maker=DAOGroup(2),
+                                    bkg_estimator=None, psf_model=prf,
+                                    fitshape=7, finder=tophatfinder)
+    # Test for init_guesses is None
+    with pytest.warns(AstropyUserWarning, match='aperture_radius is None and '
+                                                'could not be determined'):
+        results = basic_phot(image=img)
+    assert_allclose(results['flux_fit'], f0, rtol=0.05)
+
+    # Have to reset the object or it saves any updates, and we wish to
+    # re-verify the aperture_radius assignment
     basic_phot = BasicPSFPhotometry(group_maker=DAOGroup(2),
                                     bkg_estimator=None, psf_model=prf,
                                     fitshape=7)
 
-    with pytest.raises(ValueError):
-        basic_phot(image=image)
+    # Test for init_guesses is not None, but lacks a flux_0 column
+    with pytest.warns(AstropyUserWarning, match='aperture_radius is None and '
+                                                'could not be determined'):
+        results = basic_phot(image=img, init_guesses=intab)
+    assert_allclose(results['flux_fit'], f0, rtol=0.05)
 
-    # with initial guesses, but without a "flux_0" column
-    intab = Table(data=[[1], [1]], names=['x_0', 'y_0'])
-    with pytest.raises(ValueError):
-        basic_phot(image=image, init_guesses=intab)
+    iter_phot = IterativelySubtractedPSFPhotometry(finder=tophatfinder,
+                                                   group_maker=DAOGroup(2),
+                                                   bkg_estimator=None,
+                                                   psf_model=prf,
+                                                   fitshape=7, niters=2)
+    # Test for init_guesses is not None, but lacks a flux_0 column
+    with pytest.warns(AstropyUserWarning, match='aperture_radius is None and '
+                                                'could not be determined'):
+        results = iter_phot(image=img, init_guesses=intab)
+    assert_allclose(results['flux_fit'], f0, rtol=0.05)
+
+    iter_phot = IterativelySubtractedPSFPhotometry(finder=tophatfinder,
+                                                   group_maker=DAOGroup(2),
+                                                   bkg_estimator=None,
+                                                   psf_model=prf,
+                                                   fitshape=7, niters=2)
+
+    # Test for init_guesses is None
+    with pytest.warns(AstropyUserWarning, match='aperture_radius is None and '
+                                                'could not be determined'):
+        results = iter_phot(image=img)
+    assert_allclose(results['flux_fit'], f0, rtol=0.05)
 
 
 @pytest.mark.skipif('not HAS_SCIPY')