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

Fix minor bugs in peak culling

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: pipe_tasks
    • Labels:
      None

      Description

      Sogo Mineo reports another bug in peak culling that makes it hard to disable via configuration:

      The docstring of class CullPeaksConfig says:

      To disable peak culling, simply set nBandsSafe=1.

      If I assume `nBandsSafe` is a typo for `nBandsSufficient`
      and set nBandsSufficient = 1,
      then the condition for a peak to be kept:

      if ((rank < self.config.cullPeaks.rankSufficient) or
      (self.config.cullPeaks.nBandsSufficient > 1 and
      sum([peak.get(k) for k in keys]) >= self.config.cullPeaks.nBandsSufficient) or
      (rank < self.config.cullPeaks.rankConsidered and
      rank < self.config.cullPeaks.rankNormalizedConsidered * familySize)):

      will be equivalent to:

      if ((rank < self.config.cullPeaks.rankSufficient) or
      (rank < self.config.cullPeaks.rankConsidered and
      rank < self.config.cullPeaks.rankNormalizedConsidered * familySize)):

      and peak culling will be still active in spite of the instruction of the docstring.

        Attachments

          Issue Links

            Activity

            Hide
            jbosch Jim Bosch added a comment -

            Paul Price, another peak-culling review for you, fixing the issue Mineo brought up.

            I'm planning to reprocess a COSMOS patch today to check out the effects of various peak-culling options and test that this now works as advertised.

            Show
            jbosch Jim Bosch added a comment - Paul Price , another peak-culling review for you, fixing the issue Mineo brought up. I'm planning to reprocess a COSMOS patch today to check out the effects of various peak-culling options and test that this now works as advertised.
            Hide
            price Paul Price added a comment -

            Looks good. Peak culling can now be disabled by setting nBandsSufficient=1.

            Show
            price Paul Price added a comment - Looks good. Peak culling can now be disabled by setting nBandsSufficient=1 .
            Hide
            jbosch Jim Bosch added a comment -

            While testing this, it seems a small number of peaks only appear when peak culling is enabled (both the broken version and the fixed version, though it's different peaks in those two cases), which completely baffles me. Probably shouldn't merge anything until I figure out what's going on.

            Show
            jbosch Jim Bosch added a comment - While testing this, it seems a small number of peaks only appear when peak culling is enabled (both the broken version and the fixed version, though it's different peaks in those two cases), which completely baffles me. Probably shouldn't merge anything until I figure out what's going on.
            Hide
            jbosch Jim Bosch added a comment -

            False alarm: the above weird peaks are sky objects.

            Show
            jbosch Jim Bosch added a comment - False alarm: the above weird peaks are sky objects.
            Hide
            jbosch Jim Bosch added a comment - - edited

            An interesting region in tract=9813 (COSMOS UltraDeep), patch=4,4. Image is i-band.
            Overplotted green markers are peaks that are culled after merging by the fixed code (i.e. post-DM-11625), constructed by comparing runs on tickets/DM-11628 with the default config and nBandsSufficient=1 (which now correctly disables peak culling). Overplotted red markers are peaks that were culled by the broken code (pre-DM-11625) but are not culled by the fixed code.

            There are between 1-3 culled peaks in this image that I'd bet are real, all at the faint end where it's easy to believe they only appear in the deepest band. And the culling seems to be helpful at getting rid of the detections from the satellite trail that leaked through in g-band (not shown, other than in the linear feature of culled peaks).

            Bottom line is that from this very preliminary look, whether to enable or disable peak-culling is going to involve some judgement call in weighing completeness vs. false positives.

            Show
            jbosch Jim Bosch added a comment - - edited An interesting region in tract=9813 (COSMOS UltraDeep), patch=4,4. Image is i-band. Overplotted green markers are peaks that are culled after merging by the fixed code (i.e. post- DM-11625 ), constructed by comparing runs on tickets/ DM-11628 with the default config and nBandsSufficient=1 (which now correctly disables peak culling). Overplotted red markers are peaks that were culled by the broken code (pre- DM-11625 ) but are not culled by the fixed code. There are between 1-3 culled peaks in this image that I'd bet are real, all at the faint end where it's easy to believe they only appear in the deepest band. And the culling seems to be helpful at getting rid of the detections from the satellite trail that leaked through in g-band (not shown, other than in the linear feature of culled peaks). Bottom line is that from this very preliminary look, whether to enable or disable peak-culling is going to involve some judgement call in weighing completeness vs. false positives.
            Hide
            jbosch Jim Bosch added a comment -

            Merged to master.

            Show
            jbosch Jim Bosch added a comment - Merged to master.

              People

              Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Paul Price
              Watchers:
              Jim Bosch, Paul Price
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: