WIP: Overview Documentation for PSF Fitting Workflow

[Onoddil]

This pull request is set up for the discussion of the overall PSF fitting process documentation, the fitting workflow and how each block interacts internally, and should not be merged until after the discussion is finished. This documentation complements that in #721 where the documentation for those blocks implemented, with solid APIs unlikely to change in future releases, and the newer APIs available in PRs #753, #755 and #756 for which details may be subject to change.

The main issues for discussion in this document are mostly limited to formatting and clarity of wording, as discussion of individual blocks should be directed to their respective PRs. The issues are summarised below:

• High Level Overview
  • Name
  • Wording
  • Formatting
  • Workflow graphic representation

One issue that should be discussed is the final graphical representation of the workflow. The current graph is simply a reproduction of the block diagram, but this PR is meant to supersede that (and closes #761), and so confirmation of the workflow represented here is merited.

Please provide any feedback on the description of the overall PSF fitting process you may have, such that the implementation meets all of the requirements of all users and is as clear as possible going forward. Attached is the final output of the graph for viewing, as currently graphviz is not viewable within this PR.

[eteq]

@Onoddil - overall this is a great "big picture" step! I have lots of inline comments, of course, but those are more details vs the whole scheme. In general this is really good.

One thing that's missing though, is a bit more on how BasicPSFPhotometry plays with the IterativelySubtracted version. That is, which parts of these are in "basic"? I'm not sure if this is best done as a "sub box" in the block diagram vs a separate diagram, but it may be useful to show that so that the user can see in pictures that they are connected, at least conceptually if not literally in the code?

Also, I think even though some of my comments rely on discussion in the other PRs, I think it's OK for us to merge this if we are happy with it and then rebase the others on that if any changes are needed. That'll let us finish things in "stages" anyway.

[eteq]

This may be better discussed in #756 ... but this discussion makes me realize that maybe the scene maker and single object model should be one thing? Or even if separate classes, maybe they should be described together in one document?

[Onoddil]

They're basically "point + extended" sources extensions of group maker and the PSF model, so while linked slightly more obviously by the need to share a common object_type list and know what to do with each available choice in each instance, they're no more or less inter-connected than group maker and PSF model; it's just obvious what to do with those as the only potential object type is a point source, which is simple to handle.

We can discuss this further in the corresponding PR to get into the details, and, again, I'll make any changes decided upon in there in this branch too.

[eteq]

This (along with the scene maker) are optional, right? That is, if not present, it means just use the PSF model directly? In that case you should say that in both descriptions.

[Onoddil]

I've added language to this effect in the text, but eventually I don't think they'll be entirely optional, in that I think the block will always have to be used, but it can have convenient "do nothing" defaults. I think it's tied to the object_type flag that will eventually have to be thrown around in the astropy.table.Table objects: if that (by default) is something like point source then Single Object Model simply returns the input PSF model (i.e., f * δ = f) and Scene Maker just returns the input table of objects (i.e., no input sources are an extended source split apart).

This effectively means they're optional, as the user can just ignore that the more complex functionality exists and default parameters unchanged give the same answer as if they weren't there, but I think that's easier than having if use_som: new_model = single_object_model(psf) or equivalent.

[marthaboyer]

@Onoddil This all looks fantastic to me! I agree with Eriks suggestions & don't really have much else to add.

[Onoddil]

Thanks @eteq and @marthaboyer for proofs! I've made most of these changes without comment as they're good suggestions, but I've replied to a few inline comments for further question.

With regards to the larger comment of how BasicPSFPhotometry interacts with IterativelySubtractedPSFPhotometry, I'm not sure how this should be tackled. I don't like that we focus so much on a single iterative fitting algorithm (as of writing it's the only one available, but see #732 for a new one...). The concept is that a "Basic PSF fit" is (see the second paragraph of this document for the basic flow, where I thought I discussed this but perhaps not obviously enough): image -> finder (optionally) -> grouping -> fitting -> sources. An "Iterative PSF fit" does those things, then goes to culler and ender, potentially interacts with image again (IterativelySubtractedPSFPhotometry, by the name, subtracts sources found in one iteration and re-fits the residuals, e.g.), potentially interacts with guess at sources (#732 stacks all previously detected sources to be re-fit together, instead of fitting sources in residuals, so it swaps image editing for output table editing), then goes back to finder... Thus one could actually view a "Basic PSF fit" as an "Iterative PSF fit" with culler and ender defined to never cull any sources but always end the 'loop'.

However, as I imagine we can't really get rid of BasicPSFPhotometry (as much as I'd like to just define a "Basic PSF fit" as "pick your favourite iterative routine but don't worry, set Culler and Ender to this BasicCullerEnder and it'll not make any difference"), I think we could better factor the codebase to use as much of BasicPSFPhotometry in subsequent subclasses as possible to highlight that it's basically just one iteration of the code. Thus while a BasicFit is image -> finder (optionally) -> grouping -> fitting -> sources, IterativePSFPhotometry is BasicFit -> culler and ender -> (maybe end or) -> mess with image -> BasicFit -> ... -> end -> sources.

As currently written I don't think it's quite so simple but I don't think the refactor to define things as such is too difficult; BasicPSFPhotometry and iterative subclasses do the heavy lifting in do_photometry but if that were changed so BasicPSFPhotometry essentially has def do_photometry(self, ...): return do_single_loop(self, ...) then subclasses could overload do_photometry but still use the super do_single_loop function.

For this documentation (instead of me waxing philosophical about what I'd like to do to the code...): the attempt to explain how IterativePSFPhotometry builds off BasicPSFPhotometry is in paragraph two. I added the box around the basic PSF parts of the block diagram, but it's basically everything except the culler and ender. I also added the part about "messing with the image" as an optional block. I think (given that I've just reproduced someone else's original block diagram) that the "subtract" floating between fitter and culler and ender was IterativelySubtractedPSFPhotometry's "subtract fit sources from image and find more in the residuals", so we should actually probably more generalise that either way, as that isn't true for all algorithms.

Let me know if the improvements to the block diagram help, and then any suggestions for how to better explain that IterativePSFPhotometry is the entirety of BasicPSFPhotometry + Culler and Ender + Tweak Input Image as a single iteration.

[Onoddil]

Hi @marthaboyer and @eteq - as per a conversation I had offline with @eteq I've pushed the changes I made in response to the comments that were straightforward, but have a few inline replies to comments for clarifications, and the more general query in the comment above. I would appreciate it if you could review the changes I already made, have a look at the queries I had to a few of the comments you had previously, and have a think about the larger issue in my reply above this one.