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

(In)equality semantics of Coords are confusing

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
      None

      Description

      Viz:

      In [1]: from lsst.afw.coord import Coord
      In [2]: c1 = Coord("11:11:11", "22:22:22")
      In [3]: c1 == c1, c1 != c1
      Out[3]: (True, False)
      In [4]: c2 = Coord("33:33:33", "44:44:44")
      In [5]: c1 == c2, c1 != c2
      Out[5]: (False, True)
      In [6]: c3 = Coord("11:11:11", "22:22:22")
      In [7]: c1 == c3, c1 != c3
      Out[7]: (True, True)

      c1 is simultaneously equal to and not equal to c3!

        Attachments

          Activity

          No builds found.
          swinbank John Swinbank created issue -
          swinbank John Swinbank made changes -
          Field Original Value New Value
          Description Viz:

          {code}
          In [1]: from lsst.afw.coord import Coord
          In [2]: c1 = Coord("11:11:11", "22:22:22")
          In [3]: c1 == c1, c1 != c1
          Out[3]: (True, False)
          In [4]: c2 = Coord("33:33:33", "44:44:44")
          In [5]: c1 == c2, c1 != c2
          Out[5]: (False, True)
          In [6]: c3 = Coord("11:11:11", "22:22:22")
          In [7]: c1 == c3, c1 != c3
          Out[7]: (True, True)
          {code}

          `c1` is simultaneously equal to *and* not equal to `c3`!
          Viz:

          {code}
          In [1]: from lsst.afw.coord import Coord
          In [2]: c1 = Coord("11:11:11", "22:22:22")
          In [3]: c1 == c1, c1 != c1
          Out[3]: (True, False)
          In [4]: c2 = Coord("33:33:33", "44:44:44")
          In [5]: c1 == c2, c1 != c2
          Out[5]: (False, True)
          In [6]: c3 = Coord("11:11:11", "22:22:22")
          In [7]: c1 == c3, c1 != c3
          Out[7]: (True, True)
          {code}

          {{c1}} is simultaneously equal to *and* not equal to {{c3}}!
          Hide
          swinbank John Swinbank added a comment -

          This issue is, of course, that we provide an operator==() for Coord, which compares by value, but no corresponding operator!=(), so that the check for inequality is done by identity.

          An easy fix is just to add something along the lines of:

          inline bool lsst::afw::coord::Coord::operator!=(lsst::afw::coord::Coord const &rhs) const {
              return !operator==(rhs);
          }

          Jim Bosch, you know this code better than I do – any reason not to go ahead and do just that?

          Show
          swinbank John Swinbank added a comment - This issue is, of course, that we provide an operator==() for Coord , which compares by value, but no corresponding operator!=() , so that the check for inequality is done by identity. An easy fix is just to add something along the lines of: inline bool lsst::afw::coord::Coord::operator!=(lsst::afw::coord::Coord const &rhs) const { return !operator==(rhs); } Jim Bosch , you know this code better than I do – any reason not to go ahead and do just that?
          Hide
          price Paul Price added a comment - - edited

          I think you want to do:

          %include "lsst/p_lsstSwig.i"
          %useValueEquality(lsst::afw::coord::Coord);

          Although I've noticed that this macro (and %usePointerEquality) contain:

                      try:
                          return self._eq_impl(rhs)
                      except Exception:
                          return NotImplemented

          i.e., in the event the equality throws an exception, they're returning a class (which always evaluates to True) instead of raising an exception instance.

          Show
          price Paul Price added a comment - - edited I think you want to do: %include "lsst/p_lsstSwig.i" %useValueEquality(lsst::afw::coord::Coord); Although I've noticed that this macro (and %usePointerEquality) contain: try: return self._eq_impl(rhs) except Exception: return NotImplemented i.e., in the event the equality throws an exception, they're returning a class (which always evaluates to True ) instead of raising an exception instance.
          Hide
          price Paul Price added a comment -

          Created DM-2348 for that.

          Show
          price Paul Price added a comment - Created DM-2348 for that.
          Hide
          swinbank John Swinbank added a comment -

          The macro is neat, but isn't comparing coordinates in C++ potentially useful as well?

          Show
          swinbank John Swinbank added a comment - The macro is neat, but isn't comparing coordinates in C++ potentially useful as well?
          Hide
          jbosch Jim Bosch added a comment - - edited

          I think we do want to add the C++ operator!=, but we should be using %useValueEquality here too to get the right Python behavior when comparing Coords to other types. I'm not sure yet whether this has revealed a problem in %useValueEquality, but I'll figure that out on DM-2348.

          Show
          jbosch Jim Bosch added a comment - - edited I think we do want to add the C++ operator!= , but we should be using %useValueEquality here too to get the right Python behavior when comparing Coords to other types. I'm not sure yet whether this has revealed a problem in %useValueEquality , but I'll figure that out on DM-2348 .
          Hide
          swinbank John Swinbank added a comment -

          The downside to that is that %useValueEquality delegates to the C++ operator== and just negates the result when checking for inequality. By also defining an operator!= in C++ we could quite easily end up with different semantics in C++ and Python, which seems unfortunate.

          (Of course, if somebody wants to define an operator!= which isn't just the complement of operator== they should probably think again, but...)

          Show
          swinbank John Swinbank added a comment - The downside to that is that %useValueEquality delegates to the C++ operator== and just negates the result when checking for inequality. By also defining an operator!= in C++ we could quite easily end up with different semantics in C++ and Python, which seems unfortunate. (Of course, if somebody wants to define an operator!= which isn't just the complement of operator== they should probably think again, but...)
          Hide
          jbosch Jim Bosch added a comment -

          I suppose we could essentially require the existence of operator!= in C++ by having %useValueEquality delegate to that for __ne__.

          Show
          jbosch Jim Bosch added a comment - I suppose we could essentially require the existence of operator!= in C++ by having %useValueEquality delegate to that for __ne__ .
          swinbank John Swinbank made changes -
          Epic Link DM-1910 [ 15942 ]
          swinbank John Swinbank made changes -
          Story Points 1
          Hide
          swinbank John Swinbank added a comment -

          I watched an entertaining rant by Alex Stepanov over the weekend, to the effect that it's basically always a design flaw if !(thing == other) is not equivalent to thing != other. I'm persuaded that I like that solution best, iff it doesn't introduce too much overhead – I'm not sure how many places we're using %useValueEquality, and I don't think it's worth spending a long time checking them down. I made the change as suggested, fixed a couple of places where I know it'll break, and submitted to buildbot – let's see what fails.

          Show
          swinbank John Swinbank added a comment - I watched an entertaining rant by Alex Stepanov over the weekend, to the effect that it's basically always a design flaw if !(thing == other) is not equivalent to thing != other . I'm persuaded that I like that solution best, iff it doesn't introduce too much overhead – I'm not sure how many places we're using %useValueEquality , and I don't think it's worth spending a long time checking them down. I made the change as suggested, fixed a couple of places where I know it'll break, and submitted to buildbot – let's see what fails.
          Hide
          swinbank John Swinbank added a comment -

          It turns out that it took only a few lines of changes in utils and afw to delegate Python's __ne__ to C++ and then provide operator!= everywhere that it's required. Given that, I think this is the approach I prefer.

          Jim, the changes are on the tickets/DM-2347 branch on utils and afw. Please check that you're happy not just with those changes but with the approach adopted. Thanks!

          Show
          swinbank John Swinbank added a comment - It turns out that it took only a few lines of changes in utils and afw to delegate Python's __ne__ to C++ and then provide operator!= everywhere that it's required. Given that, I think this is the approach I prefer. Jim, the changes are on the tickets/ DM-2347 branch on utils and afw . Please check that you're happy not just with those changes but with the approach adopted. Thanks!
          swinbank John Swinbank made changes -
          Reviewers Jim Bosch [ jbosch ]
          Status To Do [ 10001 ] In Review [ 10004 ]
          Hide
          jbosch Jim Bosch added a comment -

          Looks good to me. I think I agree that's the best approach (I certainly agree that everything that implements == should implement != in C++; I'm ambivalent as to how we enforce that).

          Show
          jbosch Jim Bosch added a comment - Looks good to me. I think I agree that's the best approach (I certainly agree that everything that implements == should implement != in C++; I'm ambivalent as to how we enforce that).
          jbosch Jim Bosch made changes -
          Status In Review [ 10004 ] Reviewed [ 10101 ]
          swinbank John Swinbank made changes -
          Resolution Done [ 10000 ]
          Status Reviewed [ 10101 ] Done [ 10002 ]

            People

            Assignee:
            swinbank John Swinbank
            Reporter:
            swinbank John Swinbank
            Reviewers:
            Jim Bosch
            Watchers:
            Jim Bosch, John Swinbank, Paul Price
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins Builds

                No builds found.