Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-903

SourceDetectionTask should only add flags.negative if config.thresholdParity == "both"

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_algorithms
    • 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

          Issue Links

            Activity

            Hide
            pgee 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
            pgee 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
            rhl 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
            rhl 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
            pgee 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
            pgee 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
            pgee 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
            pgee 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
            pgee 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
            pgee 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
            pgee 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
            pgee 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
            jbosch 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
            jbosch 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
            rhl 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
            rhl 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
            jbosch 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
            jbosch 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
            rhl 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
            rhl 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
            jbosch 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
            jbosch 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
            rhl 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
            rhl 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
            jbosch 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
            jbosch 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
            pgee 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
            pgee 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
            jbosch 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
            jbosch 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:
                pgee Perry Gee
                Reporter:
                rhl Robert Lupton
                Reviewers:
                Jim Bosch
                Watchers:
                Jim Bosch, John Swinbank, Perry Gee, Robert Lupton
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: