DM-46697: Rework the run signature for make_direct_warp

[arunkannawadi]

No description provided.

[TallJimbo]

I'm not totally thrilled with the idea that we'll be burning even a small number of compute cycles by adding zeros to images, but if this helps a lot in reducing complexity it's probably still worth it. Maybe after this ticket we could add a "apply" and "unapply" images to BackgroundList itself, and implement them to do nothing when the list is empty such that you could use a trivial BackgroundList instead. If that sounds good, please make a ticket.

Either way, it might be good to give this class a leading-underscore name, since it doesn't seem to be used outside of method bodies and type annotations (which are not so important here in pipe_tasks at present).

[TallJimbo]

Another option might be to drop this class, allow process to accept None for its background list arguments regardless of what the config options are, and move the check for consistency between input arguments and configuration up to run.

[arunkannawadi]

I certainly didn't like the way the code read when I had to check if the backgrounds were None before calling getImage, but I am on board with any of the solutions here. I'll give it some more thought and am happy to just add it on this ticket.

[arunkannawadi]

Alright, this is what I'm inclining towards:

1. Keep the TrivialBackgroundList class, but with a leading underscore name and drop the getImage method.
2. Add apply and unapply methods (names TBD) to WarpDetectorInputs, since the operations happen on the components of that dataclass. These would do raise a RuntimeError if the background is None (current behavior), do nothing if the background is a _TrivialBackgroundList and do addition/subtraction only otherwise. It has the advantage of keeping track of whether we are dealing with background subtracted/added image or not.

[TallJimbo]

(preexisting, below): I think we can drop the calexpType connection template, and save ourselves a deprecation cycle for the config field it creates later.

[arunkannawadi]

I'm not sure I follow the second part of this comment - what config field does this create?

[TallJimbo]

A connection template creates a config field alongside all the collection-name ones, e.g. config.connections.calexpType.

[TallJimbo]

(preexisting, many lines above): extra newline after the opening triple-quote for MAX_NUMBER_OF_NOISE_REALIZATIONS. But I also wonder whether it's worthwhile to have a config limit on the maximum.

[TallJimbo]

Here: seems like bgSubtracted should also depend on doApplyNewBackground as well, which might make being able to assign to it tricky. Or if the assertion is that doApplyNewBackground=True requires doRevertOldBackground=False (because the skyCorr always includes a revert of the calexp background internally), we should add comment to that effect here and check it validate.

[arunkannawadi]

Having a non-configurable maximum limit is somewhat intentional, to prevent too many noise realizations from being generated accidentally. I'd rather just let the numberOfNoiseRealizations take any non-negative integer value than setting a configurable maximum limit.

[TallJimbo]

The i.e. parenthetical here seems out of place; should the example now be noise realizations or mask fractions instead?

[TallJimbo]

This is an elif on the same condition as the original if. Is it supposed to be an else? I'm not actually sure it should exist, because if useVisitSummaryPsf is False, we should just not try to get a PSF from the visit summary.

[TallJimbo]

Yes, it should happen even if visit_summary is None.