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

Invalid memory access for getX/getY when slots aren't defined

    XMLWordPrintable

    Details

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

      Description

      Looks like we have an out-of-range memory access error hiding in the catalog centroid getters, if the catalog does not have centroid slots defined:

      import lsst.afw.table
      		 
      schema = lsst.afw.table.SourceTable.makeMinimalSchema()
      		 
      lsst.afw.table.Point2DKey.addFields(schema, "centroid", "centroid", "pixels")
      		 
      catalog = lsst.afw.table.SourceCatalog(schema)
      		 
      record = catalog.addNew()
      		 
      record.set('centroid_x', 1)
      		 
      record.set('centroid_y', 2)
      		 
      record = catalog.addNew()
      		 
      record.set('centroid_x', 4)
      		 
      record.set('centroid_y', 5)
      		 
      # no slots defined results in nonsense!
      		 
      print(catalog.getX())
      		 
      print(catalog.getY())

      prints various nonsense values, like:

      [1.265e-321 2.846e-321]
      		 
      [1.265e-321 2.846e-321]

      We should probably check the other getters for similar bugs.

        Attachments

          Issue Links

          relates to

          Bug - A problem which impairs or prevents the functions of the product. DM-17452 slots are not propagated into MultiMatch output schema

          • Done

            Activity

            Ascending order - Click to sort in descending order
            5 older comments
            Hide
            Permalink
            swinbank John Swinbank added a comment -

            Thank you for for the review!

            I've responded to comments on the GitHub PR, and added a few more comments to the code.

            A few points worth noting:

            • I think I disagree with you about the desirability of assertRaises as a context manager (I think this is mostly a matter of taste: in some cases, I find it helps; in others, not so much).
            • I quite like having the tests in a separate commit to the code, although I don't feel strongly about this.
            • I agree that testColumnView and _testFluxSlot are too crowded. However, I think my additions to them are in keeping with the (overcrowded!) style of the rest of the test suite. I'd be sympathetic to a request that we should ticket a refactoring of this whole suite (on the other hand, I'm not sure we'd ever actually be in a position to devote resources to that, so I'm not sure it's worth the effort).

            If any of the above make you scream, shout and I'll change my mind before merging; otherwise, I'll push this in as it now is sometime tomorrow.

            Show
            swinbank John Swinbank added a comment - Thank you for for the review! I've responded to comments on the GitHub PR, and added a few more comments to the code. A few points worth noting: I think I disagree with you about the desirability of assertRaises as a context manager (I think this is mostly a matter of taste: in some cases, I find it helps; in others, not so much). I quite like having the tests in a separate commit to the code, although I don't feel strongly about this. I agree that testColumnView and _testFluxSlot are too crowded. However, I think my additions to them are in keeping with the (overcrowded!) style of the rest of the test suite. I'd be sympathetic to a request that we should ticket a refactoring of this whole suite (on the other hand, I'm not sure we'd ever actually be in a position to devote resources to that, so I'm not sure it's worth the effort). If any of the above make you scream, shout and I'll change my mind before merging; otherwise, I'll push this in as it now is sometime tomorrow.
            Hide
            Permalink
            Parejkoj John Parejko added a comment -

            The only one of those I feel particularly strongly about is this one. What is your rationale for it?

            I quite like having the tests in a separate commit to the code, although I don't feel strongly about this.

            If anything, the tests should be in a commit before the fix, so that it is easy to demonstrate that the tests all fail without the fix in place. Baring that, putting them in the same commit as the fix is useful to at least make it very clear what the point of the new tests is, and to demonstrate the conditions under which the unfixed code would have failed.

            Show
            Parejkoj John Parejko added a comment - The only one of those I feel particularly strongly about is this one. What is your rationale for it? I quite like having the tests in a separate commit to the code, although I don't feel strongly about this. If anything, the tests should be in a commit before the fix, so that it is easy to demonstrate that the tests all fail without the fix in place. Baring that, putting them in the same commit as the fix is useful to at least make it very clear what the point of the new tests is, and to demonstrate the conditions under which the unfixed code would have failed.
            Hide
            Permalink
            tjenness Tim Jenness added a comment -

            The dev guide does say to use the context manager with assertRaises.

            Show
            tjenness Tim Jenness added a comment - The dev guide does say to use the context manager with assertRaises.
            Hide
            Permalink
            swinbank John Swinbank added a comment -

            The dev guide does say to use the context manager with assertRaises.

            Oh, yuck; those are very poor guidelines — far too unnecessarily prescriptive to be helpful. Still, rules is rules. Thanks Tim.

            Show
            swinbank John Swinbank added a comment - The dev guide does say to use the context manager with assertRaises. Oh, yuck; those are very poor guidelines — far too unnecessarily prescriptive to be helpful. Still, rules is rules. Thanks Tim.
            Hide
            Permalink
            swinbank John Swinbank added a comment -

            Changed the order of the commits to make John happy; changed the use of asserRaises to make the Dev Guide happy; kept hitting rebuild until eventually Jenkins was happy; merged and done.

            Show
            swinbank John Swinbank added a comment - Changed the order of the commits to make John happy; changed the use of asserRaises to make the Dev Guide happy; kept hitting rebuild until eventually Jenkins was happy; merged and done.

              People

              Assignee:
              swinbank John Swinbank
              Reporter:
              Parejkoj John Parejko
              Reviewers:
              John Parejko
              Watchers:
              Jim Bosch, John Parejko, John Swinbank, Paul Price, Russell Owen, Tim Jenness, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.