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

Fix __eq__ for defects class

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_algorithms
    • Labels:
      None

      Description

      Zipping the bboxes only works if objects are same length, so check that first.

        Attachments

          Issue Links

            Activity

            Hide
            tjenness Tim Jenness added a comment -

            Looks fine to me. Either that or use zip_longest. I think a rebase is needed.

            Show
            tjenness Tim Jenness added a comment - Looks fine to me. Either that or use zip_longest. I think a rebase is needed.
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment - - edited

            I think I prefer the explicit check, mainly because otherwise I think you end up having to check for the raise of AttributeError in order to return False, because you don't want the __eq__ operator raising rather than returning False, unless I've missed a trick (I only realised this when switching to try what you'd said).

            Thanks for the rebase prompt, I actually had forgotten that! (though I hope Jenkins & Travis wouldn't have)

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - - edited I think I prefer the explicit check, mainly because otherwise I think you end up having to check for the raise of AttributeError in order to return False, because you don't want the __eq__ operator raising rather than returning False, unless I've missed a trick (I only realised this when switching to try what you'd said). Thanks for the rebase prompt, I actually had forgotten that! (though I hope Jenkins & Travis wouldn't have)
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            Also, and this is probably beyond the scope of what I wanted to get into here, but in principle I think defects should be equal regardless of the order of the bboxes internally, just so long as their sets of pixels are the same. I tried adding this but lists of bboxes are not sortable with sorted() as they don't have an < operator, and I couldn't be bothered to work around that and so didn't put that in, but I think it might be worth considering as it's only a matter of time until that bites someone in exactly the same way subsets of defects just bit me. (And I realise it's quite likely to be me...)

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - Also, and this is probably beyond the scope of what I wanted to get into here, but in principle I think defects should be equal regardless of the order of the bboxes internally, just so long as their sets of pixels are the same. I tried adding this but lists of bboxes are not sortable with sorted() as they don't have an < operator, and I couldn't be bothered to work around that and so didn't put that in, but I think it might be worth considering as it's only a matter of time until that bites someone in exactly the same way subsets of defects just bit me. (And I realise it's quite likely to be me...)
            Hide
            tjenness Tim Jenness added a comment -

            I agree. Given that BoxI is mutable and can't be placed into a set, I think there are two options:

            • Retrieve the coordinates of the Box2Is and store them as tuples in a set for comparison.
            • Convert the defects to a mask and compare masks.
            Show
            tjenness Tim Jenness added a comment - I agree. Given that BoxI is mutable and can't be placed into a set, I think there are two options: Retrieve the coordinates of the Box2Is and store them as tuples in a set for comparison. Convert the defects to a mask and compare masks.
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            OK, I've merged this for now, and we can think about this.

            I think which of your two approaches is right depends on what is desired given the following:
            If one had two boxes totally touching in one defect set, and in the second that was a single box enclosing those two, would we want that to be equal or not? If so, then the second; if not then the first.

            Technically they wouldn't be equal, but for most practical purposes I would think they would be. Thoughts?

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - OK, I've merged this for now, and we can think about this. I think which of your two approaches is right depends on what is desired given the following: If one had two boxes totally touching in one defect set, and in the second that was a single box enclosing those two, would we want that to be equal or not? If so, then the second; if not then the first. Technically they wouldn't be equal, but for most practical purposes I would think they would be. Thoughts?
            Hide
            tjenness Tim Jenness added a comment -

            I don't think I have an opinion on the difference. You may have to ask for input from interested parties.

            Show
            tjenness Tim Jenness added a comment - I don't think I have an opinion on the difference. You may have to ask for input from interested parties.
            Hide
            mfisherlevine Merlin Fisher-Levine added a comment -

            Have filed DM-19901 for the next bit.

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - Have filed DM-19901 for the next bit.

              People

              • Assignee:
                mfisherlevine Merlin Fisher-Levine
                Reporter:
                mfisherlevine Merlin Fisher-Levine
                Reviewers:
                Tim Jenness
                Watchers:
                John Swinbank, Merlin Fisher-Levine, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel