As far as I can tell, the conditions under which flags.negative is set is actually:

schema is not None and (self.config.reEstimateBackground or self.config.thresholdPolarity != "positive"):

This seems like a rather complex condition for the init code to be checking (that is, at the time that flags.negative is added to the schema).

I think it probably needs to be that complex.

1. We can't add to a schema if it doesn't exist
2. We use a symmetric detection stage when checking the background
3. if the polarity is both or negative we need the bits set

You might want to check that I'm right about the reEstimateBackground

John Swinbank, my comment is really a style question. When there is some simple config item which decides whether or not to create a schema item, it is straightforward enough to condition the creation code on that config item. In this case, the actual use of flags.negative is slightly more complex.

The way the code in detectFootprints.py is written, a negative detection is done if either polarity != "positive" or reEstimateBackground==True. But that doesn't mean that we have to always save flags.negative to the table whenever reEstimateBackground==True.

The code does not attempt to set the flags.negative field unless it is actually in the schema, so it is safe to only create the field if polarity != "positive".

I propose that we go this way, if there are no objections.

@jbosch, can you answer this question?

In meas_algorithms, the run method of SourceDetectionTasks calls detectFootprints:

fpSets = self.detectFootprints(exposure=exposure, doSmooth=doSmooth, sigma=sigma, clearMask=clearMask)

Note that detectFootprints creates an fpSets.negative if self.config.reEstimateBackground or self.config.thresholdPolarity != "positive":

So what if self.config.reEstimateBackground==True and self.config.thresholdPolarity == "positive"? Should there still be an entry in the sources table for the negative sources?

Note the later code in the run method:

sources = afwTable.SourceCatalog(table)

table.preallocate(fpSets.numPos + fpSets.numNeg) # not required, but nice

if fpSets.negative:

    fpSets.negative.makeSources(sources)

    if self.negativeFlagKey:

        for record in sources:

        record.set(self.negativeFlagKey, True)

I think we want to fully decouple (from the perspective of the caller) the question of whether we're re-estimating the background and the question of whether we're detecting negative sources. That means:

A) Add the "negative" flag to the schema if and only if self.config.thresholdPolarity=="both".

B) If self.config.thresholdPolarity=="positive", do not add negative sources to the output catalog, and do not set any flags on the (positive) sources that are added.

C) If self.config.thresholdPolarity=="negative, do not add positive sources to the output catalog, and do not set any flags on the (negative) sources that are added.

Note that if self.config.reEstimateBackground is true in case B or C, we'll actually detect both kinds of sources, but we'll just discard the ones that weren't requested.

That seems simpler, but it means that if the user is only detecting positive and negative to make the clipping symmetrical they still need to add the negative flag although it is never set.

An alternative would be to have the code check for negative and only set it if it's in the schema.

if the user is only detecting positive and negative to make the clipping symmetrical

in reEstimateBackground you mean? I think my proposal would not require the negative flag to be added in that case.

But if what you really want is to allow the schema passed to the ctor to be None regardless of the value of thresholdPolarity, and only add the flag iff thresholdPolarity=="both" and schema is not None, that'd be fine with me, too.

reEstimateBackground uses both, but negative should not be added, and I thought that that was contrary to your proposal.

I wasn't thinking of supporting schema == None, but checking for schema.get("flags_negative") (or whatever the syntax is). If reEstimateBackground can pass schema=None that'd be OK too.

Right now the SourceDetectionTask constructor is responsible for adding flags_negative to the schema. So your proposal is that we make that the responsibility of the caller?

Hmm, I guess I am, but not intentionally. This ticket was about keeping it the responsibility of SourceDetectionTask, and I think that that requires the logic that was in the original ticket.

I'd be OK with the background code calling SourceDetectionTask with schema==None instead.

Ok, letting schema=None be valid in all cases and adding the flag field to the schema if and only if schema is not None and thresholdPolarity="both" sounds like the easiest way forward here.

I followed the last suggestion, though it was not clear to me that the flag should be set for thresholdPolarity=="negative".

Very short review of tickets/DM-903 on meas_algorithms: detection.py

Looks good! Sorry the review took me so long.

Not creating the flag for thresholdPolarity=='negative' seems correct to me - in that case all objects would have the same flag set, and hence it'd be pointless. In any case I think detecting only negative objects is quite rare; I'd be surprised if we use it all in production.