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

convert afw::table unit tests to version 1

    XMLWordPrintable

    Details

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

      Description

      Most afw::table unit tests explicitly set version 0. We should change these to test the new behaviors, not the deprecated ones.

        Attachments

          Issue Links

            Activity

            Hide
            pgee Perry Gee added a comment -

            I had make a last minute change, as I was using (and improving) the meas_base Centroid and ShapeUtilities, without thinking that I couldn't actually use them in afw unit tests.

            I fixed some of the problem using the Point2D and Quadrupole functor keys, but there is not really a great solution for the errors. The CentroidResult, ShapeResult and their functor keys are really needed to do comparisons of the Slot error results and the results read directly from the fields. I cobbled together a solution for the testSourceTable tests, but it was not as clean as using the ResultKeys from meas_base. They allow the user to get the error values back as a CovarianceMatrix, no matter how they are stored in the table. I think this capability should be in afw, not in meas_base, but I will let Jim figure this out.

            I will not seek another review, but if anyone wants to do one, I will not merge until tomorrow night. Changes are only in afw now. u/pgee/DM-1076

            Show
            pgee Perry Gee added a comment - I had make a last minute change, as I was using (and improving) the meas_base Centroid and ShapeUtilities, without thinking that I couldn't actually use them in afw unit tests. I fixed some of the problem using the Point2D and Quadrupole functor keys, but there is not really a great solution for the errors. The CentroidResult, ShapeResult and their functor keys are really needed to do comparisons of the Slot error results and the results read directly from the fields. I cobbled together a solution for the testSourceTable tests, but it was not as clean as using the ResultKeys from meas_base. They allow the user to get the error values back as a CovarianceMatrix, no matter how they are stored in the table. I think this capability should be in afw, not in meas_base, but I will let Jim figure this out. I will not seek another review, but if anyone wants to do one, I will not merge until tomorrow night. Changes are only in afw now. u/pgee/ DM-1076
            Hide
            jbosch Jim Bosch added a comment -

            Changes look fine. I'd have thought you'd be able to use a CovarianceMatrixKey for the errors, and if you tried it, I'd be curious to hear why it didn't work. But I don't think it's worth changing here, since you already have the test code working and those particular tests may not last past DM-1764 anyway.

            Show
            jbosch Jim Bosch added a comment - Changes look fine. I'd have thought you'd be able to use a CovarianceMatrixKey for the errors, and if you tried it, I'd be curious to hear why it didn't work. But I don't think it's worth changing here, since you already have the test code working and those particular tests may not last past DM-1764 anyway.
            Hide
            pgee Perry Gee added a comment -

            I thought about using the CovarianceMatrixKey, but I was having trouble constructing one of a specific type from Python. Probably just driver error – I didn't try very hard to figure it out, particularly since you had already said that I should just do enough to save the test.

            I was confused by CovarianceMatrixKey anyway. Now looking it over, I guess that the right way to construct it is to create the keys myself, and in this case, pass the constructor as an array of SigmaKeys. I kind of remember doing this in C++ when you first introduced the class. I think I was expecting it to have an addFields which was as smart as CentroidResultKey or ShapeResultKey.

            Show
            pgee Perry Gee added a comment - I thought about using the CovarianceMatrixKey, but I was having trouble constructing one of a specific type from Python. Probably just driver error – I didn't try very hard to figure it out, particularly since you had already said that I should just do enough to save the test. I was confused by CovarianceMatrixKey anyway. Now looking it over, I guess that the right way to construct it is to create the keys myself, and in this case, pass the constructor as an array of SigmaKeys. I kind of remember doing this in C++ when you first introduced the class. I think I was expecting it to have an addFields which was as smart as CentroidResultKey or ShapeResultKey.
            Hide
            jbosch Jim Bosch added a comment -

            Ah, I'd forgotten that it didn't have an addFields. Makes sense that would cause problems; I should probably add that at some point.

            Show
            jbosch Jim Bosch added a comment - Ah, I'd forgotten that it didn't have an addFields. Makes sense that would cause problems; I should probably add that at some point.
            Hide
            pgee Perry Gee added a comment -

            Yes, that was the problem with the CovarianceMatrixKey. I did not correctly call the constructor. I thought it might be a Swig problem when I couldn't construct the CovarianceMatrix3fKey.

            Anyway, the upshot is that an additional ErrorKey layered on top which is smart enough to create and manage the fields as well as recognize the difference between SIGMA_ONLY and FULL_COVARIANCE might be a useful addition.

            Show
            pgee Perry Gee added a comment - Yes, that was the problem with the CovarianceMatrixKey. I did not correctly call the constructor. I thought it might be a Swig problem when I couldn't construct the CovarianceMatrix3fKey. Anyway, the upshot is that an additional ErrorKey layered on top which is smart enough to create and manage the fields as well as recognize the difference between SIGMA_ONLY and FULL_COVARIANCE might be a useful addition.

              People

              Assignee:
              pgee Perry Gee
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Lauren MacArthur
              Watchers:
              Jim Bosch, Kian-Tat Lim, Lauren MacArthur, Paul Price, Perry Gee
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.