Details

    • Type: Story
    • Status: Done
    • Priority: Major
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_base
    • Labels:
      None
    • Templates:
    • Story Points:
      8
    • Epic Link:
    • Sprint:
      DRP F16-1, DRP F16-2, DRP F16-3
    • Team:
      Data Release Production

      Description

      In DM-4882, we observed a number of centroids measured while running the lsst_dm_stack_demo routines fall outside their associated Footprints. This was seen with both the NaiveCentroid and the SdssCentroid centroiders.

      For the purposes of DM-4882 we quieted the warnings arising from this, but we should investigate why this is happening and, if necessary, weed out small Footprints entirely.

        Issue Links

          Activity

          Hide
          wmwood-vasey Michael Wood-Vasey added a comment -

          This behavior is also seen in running the examples/runCfhtTest.sh and examples/runDecamTest.sh from validate_drp. I have not looked into details about which objects have small Footprints.

          Show
          wmwood-vasey Michael Wood-Vasey added a comment - This behavior is also seen in running the examples/runCfhtTest.sh and examples/runDecamTest.sh from validate_drp . I have not looked into details about which objects have small Footprints .
          Hide
          swinbank John Swinbank added a comment -

          Nate Lust will add some commentary here on his & Jim Bosch's thinking to date on this issue. Thanks Nate!

          Show
          swinbank John Swinbank added a comment - Nate Lust will add some commentary here on his & Jim Bosch 's thinking to date on this issue. Thanks Nate!
          Hide
          pgee Perry Gee added a comment - - edited

          Jim Bosch, Nate Lust Do we have a specific source id where this occurs? Or do I need to run the lsst_dm_stack_demo and fish around?

          Show
          pgee Perry Gee added a comment - - edited Jim Bosch , Nate Lust Do we have a specific source id where this occurs? Or do I need to run the lsst_dm_stack_demo and fish around?
          Hide
          swinbank John Swinbank added a comment -

          Reminder to Nate Lust that he owes us some commentary on this issue. Hopefully, in the process, he'll address Perry's question.

          Show
          swinbank John Swinbank added a comment - Reminder to Nate Lust that he owes us some commentary on this issue. Hopefully, in the process, he'll address Perry's question.
          Hide
          swinbank John Swinbank added a comment -

          Further reminder Nate Lust! Be good to get something in here before you disappear to SBAG...

          Show
          swinbank John Swinbank added a comment - Further reminder Nate Lust ! Be good to get something in here before you disappear to SBAG...
          Hide
          nlust Nate Lust added a comment -

          So the plan that I remember discussing with Jim Bosch was to create some utility code which all centroiders could used to check if their determined centroid fell outside the footprint, and if so set the centroid value to the peak of the footprint. There also probably should be a flag that gets set when this happens, such that if a downstream plugin really needs an accurate center, it will know there was an issue. When I investigated the specific sources that caused this issue, I found that all the sources were basically junk sources, such that they were detected, but that no centroider could ever get a reasonable center for. We should not throw these out however, as they may be low surface brightness galaxies that could possibly emerge as a useful source in a coadd, hence the desire to still determine some useful center, but warn that it may not be optimal.

          Show
          nlust Nate Lust added a comment - So the plan that I remember discussing with Jim Bosch was to create some utility code which all centroiders could used to check if their determined centroid fell outside the footprint, and if so set the centroid value to the peak of the footprint. There also probably should be a flag that gets set when this happens, such that if a downstream plugin really needs an accurate center, it will know there was an issue. When I investigated the specific sources that caused this issue, I found that all the sources were basically junk sources, such that they were detected, but that no centroider could ever get a reasonable center for. We should not throw these out however, as they may be low surface brightness galaxies that could possibly emerge as a useful source in a coadd, hence the desire to still determine some useful center, but warn that it may not be optimal.
          Hide
          jbosch Jim Bosch added a comment -

          Your recollection of the plan matches mine except for one detail: instead of just checking whether the Peak is within the Footprint, we should check whether it is with a very small distance (perhaps one or two pixels?) of the peak. I'd like to also catch the case where the centroid is worse than the Peak but not bad enough to be outside the Footprint.

          Show
          jbosch Jim Bosch added a comment - Your recollection of the plan matches mine except for one detail: instead of just checking whether the Peak is within the Footprint, we should check whether it is with a very small distance (perhaps one or two pixels?) of the peak. I'd like to also catch the case where the centroid is worse than the Peak but not bad enough to be outside the Footprint.
          Hide
          pgee Perry Gee added a comment -

          I'm not sure which flag should be set on the record objects. Is this a flag of the centroider algorithm? So that every centroider algorithm which is defined in plugins has one of these flags? And is the failure for the centroid to be inside the footprint actually a FAILURE, or is it a flag outside the normal MeasurementError flags.

          It also seems like the flag should be visible at the schema (i.e., centroid slot level) Is there an alias which needs to be defined for this to work?

          Show
          pgee Perry Gee added a comment - I'm not sure which flag should be set on the record objects. Is this a flag of the centroider algorithm? So that every centroider algorithm which is defined in plugins has one of these flags? And is the failure for the centroid to be inside the footprint actually a FAILURE, or is it a flag outside the normal MeasurementError flags. It also seems like the flag should be visible at the schema (i.e., centroid slot level) Is there an alias which needs to be defined for this to work?
          Hide
          swinbank John Swinbank added a comment -

          Jim Bosch:

          instead of just checking whether the Peak is within the Footprint, we should check whether it is with a very small distance (perhaps one or two pixels?) of the peak. I'd like to also catch the case where the centroid is worse than the Peak but not bad enough to be outside the Footprint.

          (I assume that should read "whether the centroid is within the Footprint")

          Can you clarify the above a little – how can one tell that the centroid is "worse than the Peak"? Do you mean it's bad if it's offset by more than a few pixels from the peak?

          Perry Gee:

          I'm not sure which flag should be set on the record objects. Is this a flag of the centroider algorithm? So that every centroider algorithm which is defined in plugins has one of these flags?

          Yes, that's my reading of Nate's text.

          And is the failure for the centroid to be inside the footprint actually a FAILURE, or is it a flag outside the normal MeasurementError flags

          I'm not sure I'm parsing that properly, but I think that the suggestion is that we should set a "centroider-fell-back-to-peak" flag, but not a general "centroider failed" flag. It's then up to downstream algorithms to check the individual flags defined by the centroider algorithms to see if the centroid is adequate for their purposes.

          I'm actually a bit nervous about this: my first reaction is that algorithms which can sensibly fall back to peaks instead of centroids should be responsible for doing that for themselves. Having the centroider do it for them seems dangerous: what if a naive algorithm developer doesn't know or remember to check the special "centroid-is-actually-a-peak" flag? Jim will be better placed to comment on what the likely consequences of falling back to the peak when a centroid isn't available are, though, so if he assures us it will always be safe I guess that's good enough.

          It also seems like the flag should be visible at the schema (i.e., centroid slot level) Is there an alias which needs to be defined for this to work?

          As I recall – and Jim Bosch will soon correct me if I'm wrong! – this will already be handled by the existing slot mechanism. No special casing needed.

          Show
          swinbank John Swinbank added a comment - Jim Bosch : instead of just checking whether the Peak is within the Footprint, we should check whether it is with a very small distance (perhaps one or two pixels?) of the peak. I'd like to also catch the case where the centroid is worse than the Peak but not bad enough to be outside the Footprint. (I assume that should read "whether the centroid is within the Footprint") Can you clarify the above a little – how can one tell that the centroid is "worse than the Peak"? Do you mean it's bad if it's offset by more than a few pixels from the peak? Perry Gee : I'm not sure which flag should be set on the record objects. Is this a flag of the centroider algorithm? So that every centroider algorithm which is defined in plugins has one of these flags? Yes, that's my reading of Nate's text. And is the failure for the centroid to be inside the footprint actually a FAILURE, or is it a flag outside the normal MeasurementError flags I'm not sure I'm parsing that properly, but I think that the suggestion is that we should set a "centroider-fell-back-to-peak" flag, but not a general "centroider failed" flag. It's then up to downstream algorithms to check the individual flags defined by the centroider algorithms to see if the centroid is adequate for their purposes. I'm actually a bit nervous about this: my first reaction is that algorithms which can sensibly fall back to peaks instead of centroids should be responsible for doing that for themselves. Having the centroider do it for them seems dangerous: what if a naive algorithm developer doesn't know or remember to check the special "centroid-is-actually-a-peak" flag? Jim will be better placed to comment on what the likely consequences of falling back to the peak when a centroid isn't available are, though, so if he assures us it will always be safe I guess that's good enough. It also seems like the flag should be visible at the schema (i.e., centroid slot level) Is there an alias which needs to be defined for this to work? As I recall – and Jim Bosch will soon correct me if I'm wrong! – this will already be handled by the existing slot mechanism. No special casing needed.
          Hide
          jbosch Jim Bosch added a comment -

          I assume that should read "whether the centroid is within the Footprint")

          Yes, sorry, that's what I meant.

          Can you clarify the above a little – how can one tell that the centroid is "worse than the Peak"? Do you mean it's bad if it's offset by more than a few pixels from the peak?

          Yes, basically. I don't know exactly what the threshold ought to be, but if a centroid wanders too far from the peak it's likely the peak is better.

          I'm not sure I'm parsing that properly, but I think that the suggestion is that we should set a "centroider-fell-back-to-peak" flag, but not a general "centroider failed" flag. It's then up to downstream algorithms to check the individual flags defined by the centroider algorithms to see if the centroid is adequate for their purposes.

          My intent was that would set both a "centroider-fell-back-to-peak" flag AND the general "centroider failed" flag. Downstream algorithms should continue to just rely on the general failure flag to indicate whether the centroid is trustworthy. We expect them to try to run regardless because the worst-case centroid (after this fix) will still be pretty good, but they may want to use the general centroid failure flag to set their own flags.

          As I recall – and Jim Bosch will soon correct me if I'm wrong! – this will already be handled by the existing slot mechanism. No special casing needed.

          The flag will be visible enough - it will be accessible through the slot's string name (e.g. "slot_Centroid_flag_usedPeak"), though we won't have a getter for that.

          Show
          jbosch Jim Bosch added a comment - I assume that should read "whether the centroid is within the Footprint") Yes, sorry, that's what I meant. Can you clarify the above a little – how can one tell that the centroid is "worse than the Peak"? Do you mean it's bad if it's offset by more than a few pixels from the peak? Yes, basically. I don't know exactly what the threshold ought to be, but if a centroid wanders too far from the peak it's likely the peak is better. I'm not sure I'm parsing that properly, but I think that the suggestion is that we should set a "centroider-fell-back-to-peak" flag, but not a general "centroider failed" flag. It's then up to downstream algorithms to check the individual flags defined by the centroider algorithms to see if the centroid is adequate for their purposes. My intent was that would set both a "centroider-fell-back-to-peak" flag AND the general "centroider failed" flag. Downstream algorithms should continue to just rely on the general failure flag to indicate whether the centroid is trustworthy. We expect them to try to run regardless because the worst-case centroid (after this fix) will still be pretty good, but they may want to use the general centroid failure flag to set their own flags. As I recall – and Jim Bosch will soon correct me if I'm wrong! – this will already be handled by the existing slot mechanism. No special casing needed. The flag will be visible enough - it will be accessible through the slot's string name (e.g. "slot_Centroid_flag_usedPeak"), though we won't have a getter for that.
          Hide
          pgee Perry Gee added a comment -

          There is a lot of contradictory stuff on this thread, but here is what I understand should happen:

          After a Centroider does its calculation and sets its CentroidResult values, it should have a utility to call which can check to see if the centroid it calculated is (1) in the footprint and/or (2) near the footprint.getPeaks()[0]. The desired condition to trigger a centroid reset is not very clear, but we can tweak that during review. I would love to know what kind of pathological cases we are trying to code against.

          If the utility routine decides to reset the centroid to a different value, it should also (1) set the general failure flag (2) set a flag such as name + "_flag_resetToPeak".

          I'm not sure whether the routine should throw with a MeasurementError, or just set the flags and the new centroid, then return.

          I don't think any aliases are needed to do what I describe. If I am not understanding correctly, please let me know.

          Show
          pgee Perry Gee added a comment - There is a lot of contradictory stuff on this thread, but here is what I understand should happen: After a Centroider does its calculation and sets its CentroidResult values, it should have a utility to call which can check to see if the centroid it calculated is (1) in the footprint and/or (2) near the footprint.getPeaks() [0] . The desired condition to trigger a centroid reset is not very clear, but we can tweak that during review. I would love to know what kind of pathological cases we are trying to code against. If the utility routine decides to reset the centroid to a different value, it should also (1) set the general failure flag (2) set a flag such as name + "_flag_resetToPeak". I'm not sure whether the routine should throw with a MeasurementError, or just set the flags and the new centroid, then return. I don't think any aliases are needed to do what I describe. If I am not understanding correctly, please let me know.
          Hide
          jbosch Jim Bosch added a comment -

          I believe you've summarized everything quite accurately. As for your remaining questions:

          • We don't really know what the pathological cases are; we just know that sometimes the centroiders (maybe just SdssCentroid, maybe GaussianCentroid too) produce results that are too far from the peak. Tracking down exactly why they're doing this doesn't have to be part of this ticket; this is more about providing safeguards.
          • I think the desired condition for a centroid reset is either a result that is more than some configurable distance from the peak, or a result outside the source footprint.
          • Whether to throw MeasurementError is indeed an implementation detail.
          • No aliases should be required.
          Show
          jbosch Jim Bosch added a comment - I believe you've summarized everything quite accurately. As for your remaining questions: We don't really know what the pathological cases are; we just know that sometimes the centroiders (maybe just SdssCentroid, maybe GaussianCentroid too) produce results that are too far from the peak. Tracking down exactly why they're doing this doesn't have to be part of this ticket; this is more about providing safeguards. I think the desired condition for a centroid reset is either a result that is more than some configurable distance from the peak, or a result outside the source footprint. Whether to throw MeasurementError is indeed an implementation detail. No aliases should be required.
          Hide
          pgee Perry Gee added a comment -

          Why I was wondering about pathological cases: because it seems to me different problem if there is only one peak and the Centroider isn't finding it, vs. if there are two peaks and the Centroider is finding some point in between.

          About MeasurementError throw: it has never been clear to me whether an algorithm is allowed to set the FAILURE flag itself, or if it is mandatory to throw out and let the framework handle the failure. So I would like a clarification on that.

          As I understand this issue, the Centroider is really completing the measurement with the help of this piece of code am adding, and should call my code and then go on to return or do whatever else it might want to do. To do that, the flags should be set but the algorithm should not throw.

          Show
          pgee Perry Gee added a comment - Why I was wondering about pathological cases: because it seems to me different problem if there is only one peak and the Centroider isn't finding it, vs. if there are two peaks and the Centroider is finding some point in between. About MeasurementError throw: it has never been clear to me whether an algorithm is allowed to set the FAILURE flag itself, or if it is mandatory to throw out and let the framework handle the failure. So I would like a clarification on that. As I understand this issue, the Centroider is really completing the measurement with the help of this piece of code am adding, and should call my code and then go on to return or do whatever else it might want to do. To do that, the flags should be set but the algorithm should not throw.
          Hide
          jbosch Jim Bosch added a comment -

          When we run the centroiders, neighbors will have been replaced by noise, so there is only one capital-P Peak in play. There may be peaks at the level of the noise, however, and those could indeed cause problems for low SNR objects.

          Algorithms are indeed allowed to set the FAILURE flag themselves. I think I agree that this is the probably the easiest way to implement this ticket; I'm not totally certain it's the only way (I'd have to look at the code to remind myself some of the details of how it works). In any case, it would be a perfectly acceptable way to implement it.

          Show
          jbosch Jim Bosch added a comment - When we run the centroiders, neighbors will have been replaced by noise, so there is only one capital-P Peak in play. There may be peaks at the level of the noise, however, and those could indeed cause problems for low SNR objects. Algorithms are indeed allowed to set the FAILURE flag themselves. I think I agree that this is the probably the easiest way to implement this ticket; I'm not totally certain it's the only way (I'd have to look at the code to remind myself some of the details of how it works). In any case, it would be a perfectly acceptable way to implement it.
          Hide
          wmwood-vasey Michael Wood-Vasey added a comment -

          Are footprints guaranteed to be convex?

          Show
          wmwood-vasey Michael Wood-Vasey added a comment - Are footprints guaranteed to be convex?
          Hide
          jbosch Jim Bosch added a comment -

          Are footprints guaranteed to be convex?

          No, but they are guaranteed to be simply-connected.

          Show
          jbosch Jim Bosch added a comment - Are footprints guaranteed to be convex? No, but they are guaranteed to be simply-connected.
          Hide
          pgee Perry Gee added a comment -

          Hope you have time to review this, as it follows what I believe to be your prescription.

          Change is in meas_base tickets/DM-4926.

          I had to change to ordering of the initialization code in the flagDecorator to get the schema create order correct.

          Show
          pgee Perry Gee added a comment - Hope you have time to review this, as it follows what I believe to be your prescription. Change is in meas_base tickets/ DM-4926 . I had to change to ordering of the initialization code in the flagDecorator to get the schema create order correct.
          Hide
          pgee Perry Gee added a comment -

          Forgot to git add the unit test. Just checked it in.

          Show
          pgee Perry Gee added a comment - Forgot to git add the unit test. Just checked it in.
          Hide
          swinbank John Swinbank added a comment -

          Hi Perry Gee – I'll leave it for Jim to comment on whether he has time to review this. Do feel free to assign it to somebody else if he doesn't, though.

          However, one thing that jumped out at me straight away is that your commit message doesn't follow project standards. That's an easy thing to fix – please could you take care of it when you have a moment?

          Show
          swinbank John Swinbank added a comment - Hi Perry Gee – I'll leave it for Jim to comment on whether he has time to review this. Do feel free to assign it to somebody else if he doesn't, though. However, one thing that jumped out at me straight away is that your commit message doesn't follow project standards . That's an easy thing to fix – please could you take care of it when you have a moment?
          Hide
          jbosch Jim Bosch added a comment -

          What's here looks quite good; I've made some comments on the PR, but nothing major; the biggest request is switching the dist parameter from int to double. The new unit test looks especially good.

          However, before we consider this complete, I think we need to modify the existing production centroid algorithms (SdssCentroid, GaussianCentroid, and probably the centroider built into SdssShape) to use the new CentroidChecker as well. Please ping me when that's done so I can review the rest.

          Show
          jbosch Jim Bosch added a comment - What's here looks quite good; I've made some comments on the PR, but nothing major; the biggest request is switching the dist parameter from int to double . The new unit test looks especially good. However, before we consider this complete, I think we need to modify the existing production centroid algorithms ( SdssCentroid , GaussianCentroid , and probably the centroider built into SdssShape ) to use the new CentroidChecker as well. Please ping me when that's done so I can review the rest.
          Hide
          pgee Perry Gee added a comment -

          Jim Bosch
          Oops, I didn't realize this had been reviewed already, since it did not change status. The change for the two Centroid algorithms is simple, because they set their result at the very end of measure(). SdssCentroid also sets its FAILURE flag to false, which is inconsistent with many of the other algorithms, which don't set their FAILURE flag except to set it to True.

          SdssShape sets its shape->x and y in an internal subroutine which also sets the moments, and uses the safeCentroid as a starting point. It seems to me a little weird to alter the x and y for this algorithm, since the moments depend on x and y.

          Show
          pgee Perry Gee added a comment - Jim Bosch Oops, I didn't realize this had been reviewed already, since it did not change status. The change for the two Centroid algorithms is simple, because they set their result at the very end of measure(). SdssCentroid also sets its FAILURE flag to false, which is inconsistent with many of the other algorithms, which don't set their FAILURE flag except to set it to True. SdssShape sets its shape->x and y in an internal subroutine which also sets the moments, and uses the safeCentroid as a starting point. It seems to me a little weird to alter the x and y for this algorithm, since the moments depend on x and y.
          Hide
          pgee Perry Gee added a comment -

          Jim Bosch Sorry I didn't get this to you yesterday. Some big confusions with my other checkins held me up.

          I did alter NaiveCentroid, SdssCentroid and GaussianCentroid. I don't know how to construct a test of the outside-of-footprint situation, but I did put a unit test for the "distance from center" capability.

          meas_base, tickets/DM-4926

          Show
          pgee Perry Gee added a comment - Jim Bosch Sorry I didn't get this to you yesterday. Some big confusions with my other checkins held me up. I did alter NaiveCentroid, SdssCentroid and GaussianCentroid. I don't know how to construct a test of the outside-of-footprint situation, but I did put a unit test for the "distance from center" capability. meas_base, tickets/ DM-4926
          Hide
          pgee Perry Gee added a comment -

          Just noticed that centroid.py in tests is broken. The test is a delta function source, and the centroid being measure is 1-off from the single point source. I will investigate. Please do review the changes already checked in.

          Show
          pgee Perry Gee added a comment - Just noticed that centroid.py in tests is broken. The test is a delta function source, and the centroid being measure is 1-off from the single point source. I will investigate. Please do review the changes already checked in.
          Hide
          jbosch Jim Bosch added a comment -

          New changes look fine, and there's no need to check back in for additional review after fixing the last problem, unless that results in a lot of new code.

          Show
          jbosch Jim Bosch added a comment - New changes look fine, and there's no need to check back in for additional review after fixing the last problem, unless that results in a lot of new code.
          Hide
          pgee Perry Gee added a comment -

          After looking very carefully at the results from the lsst demo, I no longer think that it is safe to check this fix in without examining the cases where the centroid is not inside the footprint. I think there are issues caused by crowding and deblending which are actually causing these odd occurrences, and they are more common that one would have expected from the description of this ticket.

          Show
          pgee Perry Gee added a comment - After looking very carefully at the results from the lsst demo, I no longer think that it is safe to check this fix in without examining the cases where the centroid is not inside the footprint. I think there are issues caused by crowding and deblending which are actually causing these odd occurrences, and they are more common that one would have expected from the description of this ticket.
          Hide
          jbosch Jim Bosch added a comment -

          Nate Lust actually looked at this a bit before you started working on this, so I don't think the fact that this is somewhat common is a surprise, and I believe he tentatively concluded that the peaks were generally better (by eye) than the centroid outputs in most cases. Do you have a few specific problem cases you could post annotated images for?

          Show
          jbosch Jim Bosch added a comment - Nate Lust actually looked at this a bit before you started working on this, so I don't think the fact that this is somewhat common is a surprise, and I believe he tentatively concluded that the peaks were generally better (by eye) than the centroid outputs in most cases. Do you have a few specific problem cases you could post annotated images for?
          Hide
          pgee Perry Gee added a comment -

          I'm not saying that the solution you asked for is not the correct one. I just wanted to take a few more hours to look over the failures.

          I also was not sure if it was still the correct procedure to check the expected result back into the master directly. And if so, how do I produce the results for DarwinX86, which are new since the last time I did this.

          Finally, there is an issue I noticed when the Centroid in question is the centroid slot. Downstream plugins which depend on the slot may fail because the general failure flag on the centroid slot may fail, the way we have implemented this. Is this your intention?

          Show
          pgee Perry Gee added a comment - I'm not saying that the solution you asked for is not the correct one. I just wanted to take a few more hours to look over the failures. I also was not sure if it was still the correct procedure to check the expected result back into the master directly. And if so, how do I produce the results for DarwinX86, which are new since the last time I did this. Finally, there is an issue I noticed when the Centroid in question is the centroid slot. Downstream plugins which depend on the slot may fail because the general failure flag on the centroid slot may fail, the way we have implemented this. Is this your intention?
          Hide
          pgee Perry Gee added a comment -

          Jim Bosch
          Did you see this comment from me?

          The small demo of 4200 has 753 failures in GaussianCentroid created by the new checker. I think that most of them are sources which are deblended children of some bright source. If you look at the footprints which get created for these children, I think that you would think the current deblender is completely nuts. Anyway, the footprints are of extremely odd shapes, and furthermore, most of the footprint has been noise replaced because of the proximity of much brighter nearby objects.

          I think this is a problem with the checker. If we are creating the footprint based on a set of pixels which are then wiped out by the noise replacer, it is too much to expect the centroid algorithm to agree with the footprint.

          The other failure case, which happens less frequently, is very small objects which mostly look like noise objects, but it is hard to tell. Looks like when the brightest pixels are at the edge of the footprint, the centroid gets detected outside.

          Show
          pgee Perry Gee added a comment - Jim Bosch Did you see this comment from me? The small demo of 4200 has 753 failures in GaussianCentroid created by the new checker. I think that most of them are sources which are deblended children of some bright source. If you look at the footprints which get created for these children, I think that you would think the current deblender is completely nuts. Anyway, the footprints are of extremely odd shapes, and furthermore, most of the footprint has been noise replaced because of the proximity of much brighter nearby objects. I think this is a problem with the checker. If we are creating the footprint based on a set of pixels which are then wiped out by the noise replacer, it is too much to expect the centroid algorithm to agree with the footprint. The other failure case, which happens less frequently, is very small objects which mostly look like noise objects, but it is hard to tell. Looks like when the brightest pixels are at the edge of the footprint, the centroid gets detected outside.
          Hide
          jbosch Jim Bosch added a comment -

          Too many junk sources and bad deblending near bright objects is indeed a known issue, and we're not going to be able to fix the root issue here. It sounds like you're arguing that this is the problem (which I'd agree with), but then you've said that you think this is a problem with the checker (which I don't understand) - the goal of the checker is just to do the best it can with (admittedly) bad inputs.

          Having downstream plugins fail because the centroider did not produce a valid result (due to this check) would be considered a feature, not a bug.

          The normal procedure for the demo is to update the results on a branch and merge to master in the same way as everything else; the only differences is that you won't be able to check it on the branch with Jenkins first (at least you couldn't last time I did a demo update), so it's a bit scary, and that you'll have to ask someone else for help updating the results file with any platforms you don't have direct access to.

          Show
          jbosch Jim Bosch added a comment - Too many junk sources and bad deblending near bright objects is indeed a known issue, and we're not going to be able to fix the root issue here. It sounds like you're arguing that this is the problem (which I'd agree with), but then you've said that you think this is a problem with the checker (which I don't understand) - the goal of the checker is just to do the best it can with (admittedly) bad inputs. Having downstream plugins fail because the centroider did not produce a valid result (due to this check) would be considered a feature, not a bug. The normal procedure for the demo is to update the results on a branch and merge to master in the same way as everything else; the only differences is that you won't be able to check it on the branch with Jenkins first (at least you couldn't last time I did a demo update), so it's a bit scary, and that you'll have to ask someone else for help updating the results file with any platforms you don't have direct access to.
          Hide
          pgee Perry Gee added a comment -

          Actually, perhaps the problem is not with the deblender, but with the footprint detection. Seems like the footprint is detected with a different image (without noise replacement), which causes the footprints of smaller objects to include pixels from brighter nearby objects. However, it does not include them all. The footprint ends up looking like part of a rectangle which includes mostly pixels which belong to the brighter neighbor..

          Show
          pgee Perry Gee added a comment - Actually, perhaps the problem is not with the deblender, but with the footprint detection. Seems like the footprint is detected with a different image (without noise replacement), which causes the footprints of smaller objects to include pixels from brighter nearby objects. However, it does not include them all. The footprint ends up looking like part of a rectangle which includes mostly pixels which belong to the brighter neighbor..
          Hide
          jbosch Jim Bosch added a comment -

          Blended objects are detected as a single Footprint for all children in the blend; it's then the deblender's job to split up the flux within those pixels so noise replacement can separate them. The deblender also shrinks the Footprints of children to only contain pixels where the child has nonzero deblended flux, but there's no expectation (as in SExtractor) that child Footprints do not overlap.

          Show
          jbosch Jim Bosch added a comment - Blended objects are detected as a single Footprint for all children in the blend; it's then the deblender's job to split up the flux within those pixels so noise replacement can separate them. The deblender also shrinks the Footprints of children to only contain pixels where the child has nonzero deblended flux, but there's no expectation (as in SExtractor) that child Footprints do not overlap.
          Hide
          pgee Perry Gee added a comment -

          This may all be true. But the footprint shrinking doesn't appear to work very will. I will send you some examples, then you will see what I mean.

          Show
          pgee Perry Gee added a comment - This may all be true. But the footprint shrinking doesn't appear to work very will. I will send you some examples, then you will see what I mean.
          Hide
          pgee Perry Gee added a comment -

          Jim BoschI sent you an example of the footprint for a child which is right next to a much brighter object. As I say, this type of situation causes the Checker to go off 753 times out of 4200 with the GaussianCentroid. I'm supposing that the peak is right at the edge of the footprint, and when a Gaussian filter is applied, it must push the centroid outside the footprint.

          If you are still confident that we have done the right thing applying this Checker, I will just wipe out the old expected sources and check in what we have already reviewed.

          I do need to know if you are comfortable with the fact that setting the failure flag on the centroid slot may disrupt downstream plugins.

          Show
          pgee Perry Gee added a comment - Jim Bosch I sent you an example of the footprint for a child which is right next to a much brighter object. As I say, this type of situation causes the Checker to go off 753 times out of 4200 with the GaussianCentroid. I'm supposing that the peak is right at the edge of the footprint, and when a Gaussian filter is applied, it must push the centroid outside the footprint. If you are still confident that we have done the right thing applying this Checker, I will just wipe out the old expected sources and check in what we have already reviewed. I do need to know if you are comfortable with the fact that setting the failure flag on the centroid slot may disrupt downstream plugins.
          Hide
          price Paul Price added a comment -

          Could one of you please upload the example here?

          Show
          price Paul Price added a comment - Could one of you please upload the example here?
          Hide
          jbosch Jim Bosch added a comment -

          I do need to know if you are comfortable with the fact that setting the failure flag on the centroid slot may disrupt downstream plugins.

          Yes, as I said above, this is considered a feature, not a bug.

          If you are still confident that we have done the right thing applying this Checker, I will just wipe out the old expected sources and check in what we have already reviewed.

          Have you seen any cases where we had a valid, well-deblended (or isolated) object that was flagged and reset even though the measured centroid (without the checker) looks like a better position for it than the peak? If not, I'm confident we've done the right thing; I think the above is the only remaining case I'm worried about.

          Show
          jbosch Jim Bosch added a comment - I do need to know if you are comfortable with the fact that setting the failure flag on the centroid slot may disrupt downstream plugins. Yes, as I said above, this is considered a feature, not a bug. If you are still confident that we have done the right thing applying this Checker, I will just wipe out the old expected sources and check in what we have already reviewed. Have you seen any cases where we had a valid, well-deblended (or isolated) object that was flagged and reset even though the measured centroid (without the checker) looks like a better position for it than the peak? If not, I'm confident we've done the right thing; I think the above is the only remaining case I'm worried about.
          Hide
          jbosch Jim Bosch added a comment - - edited

          Paul Price Here are the attachments: the measurement image for a child source and an image of its Footprint. It's clear the deblender and/or detection is doing a bad job here, but I don't think it's qualitatively different from the bad deblends we see frequently around bright objects in HSC.

          Show
          jbosch Jim Bosch added a comment - - edited Paul Price Here are the attachments: the measurement image for a child source and an image of its Footprint. It's clear the deblender and/or detection is doing a bad job here, but I don't think it's qualitatively different from the bad deblends we see frequently around bright objects in HSC.
          Hide
          price Paul Price added a comment -

          It's hard to see what's going on from the footprint and noise-replaced image, and not knowing the centroid. But if you say it's fine I believe you.

          Show
          price Paul Price added a comment - It's hard to see what's going on from the footprint and noise-replaced image, and not knowing the centroid. But if you say it's fine I believe you.
          Hide
          pgee Perry Gee added a comment -

          Here is what I found after examining many of the failing centroids.

          1. Most are small sources which have been deblended from bright sources. While some of them might be real, there is a tendency around bright source to have the shadow of the bright source create noise peaks. I think for the most part, this is what we are seeing. Deblended source, not real, which do not have a compact structure. The weird footprints, while disturbing, are probably not the cause of the problem with these centroids.

          2. I also see some possibly real sources that are not deblended children. These are outside the footprint because the footprint is very small, and many of them lie outside the footprint by a fraction of a pixel. I think it is better in these case to count them as included in the footprint, so I changed the code to grow the footprint by 1 pixel.

          If anyone wants to review these changes, please respond on this issue in the next day.

          Show
          pgee Perry Gee added a comment - Here is what I found after examining many of the failing centroids. 1. Most are small sources which have been deblended from bright sources. While some of them might be real, there is a tendency around bright source to have the shadow of the bright source create noise peaks. I think for the most part, this is what we are seeing. Deblended source, not real, which do not have a compact structure. The weird footprints, while disturbing, are probably not the cause of the problem with these centroids. 2. I also see some possibly real sources that are not deblended children. These are outside the footprint because the footprint is very small, and many of them lie outside the footprint by a fraction of a pixel. I think it is better in these case to count them as included in the footprint, so I changed the code to grow the footprint by 1 pixel. If anyone wants to review these changes, please respond on this issue in the next day.
          Hide
          price Paul Price added a comment -

          Would turning on detection.doTempLocalBackground help make these cases disappear?

          Show
          price Paul Price added a comment - Would turning on detection.doTempLocalBackground help make these cases disappear?
          Hide
          jbosch Jim Bosch added a comment -

          Would turning on detection.doTempLocalBackground help make these cases disappear?

          Probably, but I think that really ought to be a separate issue.

          Show
          jbosch Jim Bosch added a comment - Would turning on detection.doTempLocalBackground help make these cases disappear? Probably, but I think that really ought to be a separate issue.
          Hide
          pgee Perry Gee added a comment -

          Jim Bosch One thing we didn't settle was whether there should be a default distance from footprint peak test. The code you reviewed does not have this test in place for any of the algorithms. Only the check for inside footprint test is automatically performed. Is that what you had in mind?

          Show
          pgee Perry Gee added a comment - Jim Bosch One thing we didn't settle was whether there should be a default distance from footprint peak test. The code you reviewed does not have this test in place for any of the algorithms. Only the check for inside footprint test is automatically performed. Is that what you had in mind?
          Hide
          jbosch Jim Bosch added a comment -

          Good question. Let's not do it just yet. I imagine a 3-pixel distance would be safe and helpful for the small, spurious objects we've focused on so far, but I'm a bit worried that this would spuriously flag some large objects with shallow profiles.

          Show
          jbosch Jim Bosch added a comment - Good question. Let's not do it just yet. I imagine a 3-pixel distance would be safe and helpful for the small, spurious objects we've focused on so far, but I'm a bit worried that this would spuriously flag some large objects with shallow profiles.
          Hide
          pgee Perry Gee added a comment -

          With GaussianCentroid, out of 4200 sources in 5 bands, the inside check eliminates 687. The maxDistToPeak < 3 eliminates 1031, and contains almost all of the failures in the first test.

          The SdssCentroid and NaiveCentroid checks eliminate less than 100 with either test.

          Show
          pgee Perry Gee added a comment - With GaussianCentroid, out of 4200 sources in 5 bands, the inside check eliminates 687. The maxDistToPeak < 3 eliminates 1031, and contains almost all of the failures in the first test. The SdssCentroid and NaiveCentroid checks eliminate less than 100 with either test.
          Hide
          jbosch Jim Bosch added a comment -

          Could you take a look at the sources eliminated by the check SdssCentroid centroid only, and just by-eye inspect the 10 with the highest SNR? If those all look like junk or cases where the centroid really did fail, I think we're good to merge.

          Show
          jbosch Jim Bosch added a comment - Could you take a look at the sources eliminated by the check SdssCentroid centroid only, and just by-eye inspect the 10 with the highest SNR? If those all look like junk or cases where the centroid really did fail, I think we're good to merge.

            People

            • Assignee:
              pgee Perry Gee
              Reporter:
              swinbank John Swinbank
              Reviewers:
              Jim Bosch
              Watchers:
              Jim Bosch, John Swinbank, Michael Wood-Vasey, Nate Lust, Paul Price, Perry Gee
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile