XMLWordPrintable

## Details

• Type: Improvement
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
1
• Sprint:
Science Pipelines DM-W15-4, Science Pipelines DM-S15-1, DRP S17-3
• Team:
Data Release Production

## Description

The SourceDetectionTask always adds "flags.negative" to the schema (if provided) but it is only used if config.thresholdParity == "both".

As adding a field to a schema requires that the table passed to the run method have that field this is a significant nuisance when reusing the task. Please change the code to only modify the schema if it's going to set it.

## Attachments

 1 Update lsst_dm_stack_demo on macOS following DM-903 Done John Swinbank

## Activity

Hide
Perry Gee added a comment -

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).

Show
Perry Gee added a comment - 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).
Hide
Robert Lupton added a comment -

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

Show
Robert Lupton added a comment - I think it probably needs to be that complex. We can't add to a schema if it doesn't exist We use a symmetric detection stage when checking the background if the polarity is both or negative we need the bits set You might want to check that I'm right about the reEstimateBackground
Hide
Perry Gee added a comment -

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.

Show
Perry Gee added a comment - 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.
Hide
Perry Gee added a comment -

Robert Lupton I think you are right about reEstimateBackground, but I will check. I'm not sure what problem with task reuse you are trying to avoid, so I am not sure if the reEstimateBackground check is needed.

Show
Perry Gee added a comment - Robert Lupton I think you are right about reEstimateBackground, but I will check. I'm not sure what problem with task reuse you are trying to avoid, so I am not sure if the reEstimateBackground check is needed.
Hide
Perry Gee added a comment -

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.

Show
Perry Gee added a comment - 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.
Hide
Perry Gee added a comment - - edited

@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) 

Show
Perry Gee added a comment - - edited @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)
Hide
Jim Bosch added a comment -

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.

Show
Jim Bosch added a comment - 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.
Hide
Robert Lupton added a comment -

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.

Show
Robert Lupton added a comment - 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.
Hide
Jim Bosch added a comment -

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.

Show
Jim Bosch added a comment - 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.
Hide
Robert Lupton added a comment -

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.

Show
Robert Lupton added a comment - 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.
Hide
Jim Bosch added a comment -

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?

Show
Jim Bosch added a comment - 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?
Hide
Robert Lupton added a comment -

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.

Show
Robert Lupton added a comment - 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.
Hide
Jim Bosch added a comment -

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.

Show
Jim Bosch added a comment - 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.
Hide
Perry Gee added a comment -

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

Show
Perry Gee added a comment - 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
Hide
Jim Bosch added a comment -

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.

Show
Jim Bosch added a comment - 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.

## People

• Assignee:
Perry Gee
Reporter:
Robert Lupton
Reviewers:
Jim Bosch
Watchers:
Jim Bosch, John Swinbank, Perry Gee, Robert Lupton