DM-25305: Add stacking of saved bright star stamps to build extended PSF model

[MorganSchmitz]

Jira ticket; Jenkins run #33904.

Main one of two PRs, the other being in daf_butler
(edit: the daf_butler PR has now been moved to DM-29583)

[leeskelvin]

typo: stackingStatistic

[leeskelvin]

Perhaps personal preference, but this could be combined into:
if bad_masks := self.config.bad_mask_planes:

[leeskelvin]

Maybe it would be cleaner to move the extra space at the end of L337 to the beginning of the f-string in L339.

[leeskelvin]

From reading this block, it seems like the purpose is to generate example_stamp. I was a little confused as to why two if statements were required, however, after having checked with you offline, it appears that it may be possible to inject a valid DeferredDatasetHandle that does not contain any brightStarStamps dataset types. In such a case however, L350 would still raise an exception and crash.

It might be more readable/cleaner to replace the two if statements with a try/except block here, i.e.:

for bss_ref in bss_ref_list:
    try:
        bss = bss_ref.get(datasetType="brightStarStamps", immediate=True)
        example_stamp = bss[0].stamp_im
        break
    except ...

or, keeping them separate still:

for bss_ref in bss_ref_list:
    try:
        bss = bss_ref.get(datasetType="brightStarStamps", immediate=True)
        break
    except ...
try:
    example_stamp = bss[0].stamp_im
except ...

or similar.

[MorganSchmitz]

Mystery solved!

This is just vestigial code left from when I had a gen2 version of this Task. The first if clause was only ever True when the inputs to run were those given by the gen2 runDataRef.
I should have removed all this (and the code in lines 367-371) when I got rid of the gen2 version. Apologies!

Note this also (sort of) explains the weird syntax I went with; I was probably cargoculting what is done in many other places in the stack when we want stuff to be able to run on both gen2 and gen3 (eg here), where the first if is really just the beginning of the handle-gen2 code block. Except in my case, I just had the one check, so it really could have been a single if statement (in fact I used to deal with this somewhat differently, and closer to what you suggested here, which is probably why I wrote things this way before switching to the syntax we see here).

Anyway - I've removed it all now, as I should have from the get go. Sorry again.

The good thing is this pushed me into testing a bunch of things, including rerunning on dataIds where I used to have trouble in gen2, and can confirm it does not occur in gen3. It also made me realize I had to file and fix DM-29862

TL;DR: this code should have never made it to the PR (it's how I used to solve a gen2-specific problem)

[leeskelvin]

As this is the second time you're checking each entry in bss_ref_list to check for bss_refs that are valid DeferredDatasetHandle types and that also contain brightStarStamps dataset types, perhaps this functionality should be stripped out and placed somewhere higher up in the run task (before L342) to check and remove invalid entries at the outset. That way, you can raise an exception and quit early if no valid entries are found, and reduce the complexity of using bss_ref_list elsewhere in the code.

[leeskelvin]

As above, this might want to be wrapped in a try/except block. (However, if the bss_ref_list checking is moved to its own initial section, then you might be safe leaving this line as-is, due to the fact that you'd then be able to guarantee this dataset type definitely already exists in bss_ref_list.