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

Make photoCalib hashable

    Details

    • Story Points:
      2
    • Team:
      Alert Production

      Description

      PhotoCalib is not hashable. As part of removing Calib, I've removed its hash function, but we now need one for PhotoCalib. It should probably delegate to the underlying BoundedField (so we'll need hash functions for those), and combine that with the hash of the mean and err.

      To first order, one could also just combine the hash of the mean and error: for any two "interesting" PhotoCalibs, those should be different, whatever the underlying BoundedFields are.

        Attachments

          Issue Links

            Activity

            Hide
            swinbank John Swinbank added a comment -

            John Parejko, Jim Bosch — what's the driver for this work? Specifically, is closing this a requirement for completion of the DM-10153 (Replace Calib with PhotoCalib) epic, or can we punt it into the future?

            Show
            swinbank John Swinbank added a comment - John Parejko , Jim Bosch — what's the driver for this work? Specifically, is closing this a requirement for completion of the DM-10153 (Replace Calib with PhotoCalib) epic, or can we punt it into the future?
            Hide
            jbosch Jim Bosch added a comment -

            I don't understand why we need a hash function for PhotoCalib, or even an equality operator.  I certainly don't see anyone using them as keys in a dict or elements in a set.

            Show
            jbosch Jim Bosch added a comment - I don't understand why we need a hash function for PhotoCalib, or even an equality operator.  I certainly don't see anyone using them as keys in a dict or elements in a set.
            Hide
            swinbank John Swinbank added a comment -

            Thanks Jim. I propose to drop this from the DM-10153 epic, and place it on the back-burner until a use case emerges.

            Show
            swinbank John Swinbank added a comment - Thanks Jim. I propose to drop this from the DM-10153 epic, and place it on the back-burner until a use case emerges.
            Hide
            Parejkoj John Parejko added a comment - - edited

            I've made use of the equality operator a few times (it already exists).

            I could see potentially tossing a bunch of PhotoCalibs into a set.

            Krzysztof Findeisen: Do you have any thoughts on this? PhotoCalib is closer to SkyWcs than most of the other things in DM-9938, even though it is immutable.

            Show
            Parejkoj John Parejko added a comment - - edited I've made use of the equality operator a few times (it already exists). I could see potentially tossing a bunch of PhotoCalibs into a set . Krzysztof Findeisen : Do you have any thoughts on this? PhotoCalib is closer to SkyWcs than most of the other things in DM-9938 , even though it is immutable.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Well, it's not clear to me that a PhotoCalib is really a value type rather than a utility class. In the former case, we should support hashability, just so that future developers don't have to waste time going "oh, it looks like I wasn't supposed to do that"; in the latter, it doesn't make sense. It sounds like John Parejko leans toward a PhotoCalib being a value.

            The main problem is a purely technical one: the way C++ handles hashing works very poorly with polymorphism, and therefore BoundedField, so supporting hashing is a lot more work than supporting equality. So based on the expense I agree with John Swinbank that we shouldn't make it a priority.

            Show
            krzys Krzysztof Findeisen added a comment - Well, it's not clear to me that a PhotoCalib is really a value type rather than a utility class. In the former case, we should support hashability, just so that future developers don't have to waste time going "oh, it looks like I wasn't supposed to do that"; in the latter, it doesn't make sense. It sounds like John Parejko leans toward a PhotoCalib being a value. The main problem is a purely technical one: the way C++ handles hashing works very poorly with polymorphism, and therefore BoundedField , so supporting hashing is a lot more work than supporting equality. So based on the expense I agree with John Swinbank that we shouldn't make it a priority.
            Hide
            jbosch Jim Bosch added a comment -

            If the use cases we've had so far are not just in unit tests, I would be interested to hear about them.  To me, PhotoCalib fits into a very common category of class (for us) in which it's super useful to have equality comparison for the purposes of writing tests, but it's never needed in any kind of regular/pipeline context.  I think it's often tempting but only sometimes productive to satisfy the need for test comparisons by adding equality to the class itself, essentially because I think the test comparisons can often afford to make simplifying assumptions that cannot be made more generally.

            Show
            jbosch Jim Bosch added a comment - If the use cases we've had so far are not just in unit tests, I would be interested to hear about them.  To me, PhotoCalib fits into a very common category of class (for us) in which it's super useful to have equality comparison for the purposes of writing tests, but it's never needed in any kind of regular/pipeline context.  I think it's often tempting but only sometimes productive to satisfy the need for test comparisons by adding equality to the class itself, essentially because I think the test comparisons can often afford to make simplifying assumptions that cannot be made more generally.
            Hide
            Parejkoj John Parejko added a comment -

            Re: equality, I think my uses were "is this persisted object the same as that other one?" From two runs of jointcal for example.

            Show
            Parejkoj John Parejko added a comment - Re: equality, I think my uses were "is this persisted object the same as that other one?" From two runs of jointcal for example.

              People

              • Assignee:
                Unassigned
                Reporter:
                Parejkoj John Parejko
                Watchers:
                Jim Bosch, John Parejko, John Swinbank, Krzysztof Findeisen
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:

                  Summary Panel