Add fallback aperture_radius to psf photometry

[Onoddil]

Added a fallback aperture_radius to BasicPSFPhotometry and IterativelySubtractedPSFPhotometry for non-Gaussian PSF models that do not have either a sigma or FWHM attribute. If neither are found, routines fall back to the smaller of the two fitshape lengths, using the available (e)PSF model pixels to compute an aperture radius flux. Closes #728

[eteq]

Big picture looks fine, a few inline suggestions.

[Onoddil]

Changes have been made as requested @eteq - please review when possible. This PR would close #804 as well as #728.

[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 See inline comments.

[larrybradley]

0.7 was released yesterday. The new changelog entry should go in 0.8 API changes.

[Onoddil]

Done; I thought this would be an issue but I put the entry where it was supposed to go at the time. It's now in the 0.8 entry with the new release, and should hopefully now reflect an update to 903 instead of replacing, but let me know if the wording isn't ok.

[larrybradley]

The logic here isn't correct. The warning will be raised despite checking if the user inputed an init_guesses table with a flux column. My deleted logic above is correct (see original lines 245-268).

[eteq]

I'm confused here. I think the intent is that the warning should not be raised if init_guesses is present and has a flux column. And that's true here, isn't it? I.e. if init_guesses is not None and 'flux_0' not in init_guesses.colnames captures "the user input an init_guesses but it doesn't have flux"?, but if the flux is there the warning is not raised. Or do you mean you want the error to be raised if init_guesses is None ?

[Onoddil]

Thanks for catching this, I did not remember to put that caveat back in. I've added this criterion back in to the else part of this to check for this again, as you had previously.

@eteq if the user supplies init_guesses which contains flux_0 then we don't need to fallback on a default aperture radius; it's only if they give init_guesses without flux_0 that we need to worry. if init_guesses is not None and 'flux_0' not in init_guesses.colnames is @larrybradley's 'deleted logic' and is what is needed, yes.

[Onoddil]

Perhaps, thinking about it, I should add some kind of test to consider these eventualities...?

[eteq]

Yes, indeed, tests for each of these cases is a very good idea! (no need to worry about the actual results, so just a one-row table or something is fine).

[larrybradley]

There are still issues here. If aperture_radius is None and the PSF model doesn't have a FWHM or sigma attribute and:

1. init_guesses is None

or

2. init_guesses is input without a flux_0 column

or

3. init_guesses is input with a flux_0 column and the PSF model doesn't have a fitshape attribute

then in each of these three cases the code will not set aperture_radius and no informative error message will be raised -- thus the code will fail exactly like it did before I added the nice error message.

We really need to write tests for each of the above cases.

[Onoddil]

I agree that the first issue is currently not covered as of the latest commit; that was a rushed job on my part to push the changes out before I went away and is my mistake, I blindly followed your suggested code instead of the intent behind your comment, and forgot to add the second criterion in, which would of course solve init_guesses is None, which I can add; thanks for spotting the missing code.

I'm not sure I follow on 2 and 3, however: 2 is covered by the code I did add to the elif catch after checking for sigma or FWHM attributes, isn't it? The docstring demands that init_guesses be an astropy.table.Table so checking for flux_0 not in init_guesses.colnames does cover option 2, surely? All I did here was use the same criterion you did, so if yours worked I can only assume mine would too...

Similarly for 3, fitshape is the routine's attribute, not the model (self.fitshape vs self.psf_model.fitshape), and cannot not be input, with the docstrings requiring int or length-2 array-like; indeed, allowing fitshape=None as an input into BasicPSFPhotometry et al. is on my to do list, and thus (for now, at least) self.fitshape is always acceptable (or the code falls over on the user before this point).

All of this is actually irrelevant, and my mistake for not realising and explaining my previous generic else catch more fully earlier: the next block of code just below this bit in BasicPSFPhotometry covers both of these eventualities: the first part covers if init_guesses is not None and flux_0 not in init_guesses.colnames while the else is therefore equivalent to if init_guesses is None. These two parts of the code block handle these issues, but simply require aperture_radius to be set, so we can just always set aperture_radius if it isn't provided (and warn the user, of course), because code already exists to account for these two issues.

I therefore actually would prefer, for the sake of clarity and code simplicity, to revert this minor change and go back to a generic else catch (with some comments -- either in the code or to the user -- to inform of why this is fine) after failing to find sigma or FWHM attributes. I will make these tests to ensure that these criteria behave as expected, of course, but with the code block already in place to handle the exceptions I think there shouldn't be any issues. If the fitting routine cannot find a sigma or FWHM attribute in self.psf_model then it falls back on an aperture_radius which is (hopefully) sensible (and gives the user to the warning to input their own, better one if it isn't), and uses that in the two cases we're worrying about, just as it would have done if the user had supplied their own aperture_radius anyway.

This actually then ties nicely into #945, as with that reverting to a generic else catch the code block for testing for the need of a fallback aperture_radius is now in agreement between BasicPSFPhotometry and IterativelySubtractedPSFPhotometry and could be split off into a private helper function as originally desired; it is the next code block in BasicPSFPhotometry that handles the additional edge case interactions with init_guesses which the _do_photometry part of IterativelySubtractedPSFPhotometry doesn't have to deal with, since it always has to do aperture photometry for its runs beyond the first and so is forced to have an aperture_radius (handling its setup equivalent to init_guesses is None all the time).

[larrybradley]

Simplifying the code with the private helper function sounds great to me! Thanks.

[larrybradley]

This will need similar logic as in BasicPSFPhotometry. Currently a warning will not be raised if init_guess is input without a 'flux' column. See block starting on new line 655. All of this logic should be the same as for BasicPSFPhotometry and should not be repeated (copy/pasted) code, but extracted into a helper function if possible.

[larrybradley]

A private helper function.

[eteq]

To add just a bit more context - @Onoddil and I discussed that probably a larger effort in this direction is called for - I made #945 to track that. But I think @larrybradley is right that we may as well start on the right foot by doing it for this specific thing, even though probably more of this is called for.

[Onoddil]

While above in BasicPSFPhotometry the logic was unsound, here it is ok I believe. For iterative PSF photometry you will need aperture_radius set here, as in _do_photometry any newly detected sources will fail with the same error when trying to calculate the aperture_flux at these lines.

These two piece of code are therefore subtly different and at this point I'm not sure you could wrap them into a helper function. Perhaps down the road once there are more iterative PSF photometry algorithms in place BasicPSFPhotometry can hold -- as the base class -- a helper function for those cases, which would basically be this code moved out into its own function, but the iterative vs 'basic' cases here are different and should be standalone, I think.

I'm not sure how this links in with the larger issue @eteq, but I think I agree overall this function eventually should become a helper function, but BasicPSFPhotometry will remain an edge case which is handled specifically in its do_photometry call.

[eteq]

Ah, I missed the subtle difference here, assuming it's true that the logic is indeed different (I admit I've lost the thread given my confusion above...). In that case I think it does make sense that they should be left separate for now.

There are some ways that one could still imagine generalizing more usefully, like having the parts inside the ifs here be wrapped up in a private function with a few parameters (and be shared in basic and iterativelysubtracted), but I think it's better to do that in a follow-on because it will probably touch a lot of places and we wouldn't want it to drag this one out further.

[larrybradley]

@Onoddil Since we already allow aperture_radius to be set by model FWHM, this is fine if fitshape can be determined.

[Onoddil]

Thanks @larrybradley, I've updated the code with your changes, and replied to each inline comment. This should be good for another review, and since it's rebased onto #942 isn't blocked anymore and should therefore be good to merge, once approved and tests pass etc. @eteq I've also replied to a few of your inline responses in the respective threads in @larrybradley's review.

[Onoddil]

Done; I thought this would be an issue but I put the entry where it was supposed to go at the time. It's now in the 0.8 entry with the new release, and should hopefully now reflect an update to 903 instead of replacing, but let me know if the wording isn't ok.

[Onoddil]

While above in BasicPSFPhotometry the logic was unsound, here it is ok I believe. For iterative PSF photometry you will need aperture_radius set here, as in _do_photometry any newly detected sources will fail with the same error when trying to calculate the aperture_flux at these lines.

These two piece of code are therefore subtly different and at this point I'm not sure you could wrap them into a helper function. Perhaps down the road once there are more iterative PSF photometry algorithms in place BasicPSFPhotometry can hold -- as the base class -- a helper function for those cases, which would basically be this code moved out into its own function, but the iterative vs 'basic' cases here are different and should be standalone, I think.

I'm not sure how this links in with the larger issue @eteq, but I think I agree overall this function eventually should become a helper function, but BasicPSFPhotometry will remain an edge case which is handled specifically in its do_photometry call.

[Onoddil]

Thanks for catching this, I did not remember to put that caveat back in. I've added this criterion back in to the else part of this to check for this again, as you had previously.

@eteq if the user supplies init_guesses which contains flux_0 then we don't need to fallback on a default aperture radius; it's only if they give init_guesses without flux_0 that we need to worry. if init_guesses is not None and 'flux_0' not in init_guesses.colnames is @larrybradley's 'deleted logic' and is what is needed, yes.

[larrybradley]

There are still issues here. If aperture_radius is None and the PSF model doesn't have a FWHM or sigma attribute and:

1. init_guesses is None

or

2. init_guesses is input without a flux_0 column

or

3. init_guesses is input with a flux_0 column and the PSF model doesn't have a fitshape attribute

then in each of these three cases the code will not set aperture_radius and no informative error message will be raised -- thus the code will fail exactly like it did before I added the nice error message.

We really need to write tests for each of the above cases.

[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

[larrybradley]

Thanks, @Onoddil.