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

Wrap leftover tests in afw with pybind11

    Details

      Description

      Wrap any afw tests in tests/test.txt that are not covered.

      This includes:

      1. testExecutables.py
      2. testLeastSquares.py
      3. testTableIO.py
      4. testVisitInfo.py
      5. testWeather.py

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment -

            This issue should not be started until all other afw wrapping issues are merged, so that it can clean up any tests that slipped through the cracks.

            Show
            krzys Krzysztof Findeisen added a comment - This issue should not be started until all other afw wrapping issues are merged, so that it can clean up any tests that slipped through the cracks.
            Hide
            rowen Russell Owen added a comment - - edited

            I think we are ready for this now.

            I found four tests that needed attention:

            • A skip in testFunction.py
            • A skip in testSpatialCell.py
            • testTableMultiMatch.py which was not in test.txt
            • testSpherePoint.py which was not in test.txt

            Tests I did not change but have skips:

            • The skip in testHeavyFootprint.py looks permanent. We should remove that test at some point, but I did not do it yet.
            • The skip in testSourceTable.py is fixed in DM-8751.
            Show
            rowen Russell Owen added a comment - - edited I think we are ready for this now. I found four tests that needed attention: A skip in testFunction.py A skip in testSpatialCell.py testTableMultiMatch.py which was not in test.txt testSpherePoint.py which was not in test.txt Tests I did not change but have skips: The skip in testHeavyFootprint.py looks permanent. We should remove that test at some point, but I did not do it yet. The skip in testSourceTable.py is fixed in DM-8751 .
            Hide
            rowen Russell Owen added a comment - - edited

            I enabled the skipped test in testFunction.py. In the process I found some errors in the C++ that I fixed (on a separate commit), including:

            • IntegerDeltaFunction1::operator() took two arguments instead of one and ignored the second. Apparently we've never needed that function, but I'd rather it be fixed now and we can decide later whether to get rid of it.
            • Many functions classes were not being instantiated. Our usual pattern is to instantiate all templated classes we expect to use in C++ or Python and apparently I messed up years ago.
            • A typo in the pybind11 wrapper rendered clone unusable for one class.
            Show
            rowen Russell Owen added a comment - - edited I enabled the skipped test in testFunction.py . In the process I found some errors in the C++ that I fixed (on a separate commit), including: IntegerDeltaFunction1::operator() took two arguments instead of one and ignored the second. Apparently we've never needed that function, but I'd rather it be fixed now and we can decide later whether to get rid of it. Many functions classes were not being instantiated. Our usual pattern is to instantiate all templated classes we expect to use in C++ or Python and apparently I messed up years ago. A typo in the pybind11 wrapper rendered clone unusable for one class.
            Hide
            rowen Russell Owen added a comment - - edited

            I enabled the skipped test in testSpatialCell.py.

            I also standardized the code in spacialCell.cc to not use pybind11/operators.h and to use namespaces in the usual way. I also added functions to declare classes because it declares so many classes. I felt it made the code significantly more readable.

            Show
            rowen Russell Owen added a comment - - edited I enabled the skipped test in testSpatialCell.py . I also standardized the code in spacialCell.cc to not use pybind11/operators.h and to use namespaces in the usual way. I also added functions to declare classes because it declares so many classes. I felt it made the code significantly more readable.
            Hide
            rowen Russell Owen added a comment - - edited

            I wrapped SpherePoint and thus fixed testSpherePoint.py.

            I fixed testTableMultiMatch.py. This test turned up several issues, including:

            • SchemaMapper.editOutputSchema was incorrectly wrapped. The returned value was not a reference, so modifying it did not have the desired affect.
            • Column views were incorrectly wrapped (every table has its own type).
            • Schema was missing a useful string representation. I added Schema.__repr__. In SWIG the __str__ was identical so I omitted it (since str is used if repr not found). I can add it if desired (and I admit we usually do, but I'm not sure why).
            Show
            rowen Russell Owen added a comment - - edited I wrapped SpherePoint and thus fixed testSpherePoint.py . I fixed testTableMultiMatch.py . This test turned up several issues, including: SchemaMapper.editOutputSchema was incorrectly wrapped. The returned value was not a reference, so modifying it did not have the desired affect. Column views were incorrectly wrapped (every table has its own type). Schema was missing a useful string representation. I added Schema.__repr__ . In SWIG the __str__ was identical so I omitted it (since str is used if repr not found). I can add it if desired (and I admit we usually do, but I'm not sure why).
            Hide
            rowen Russell Owen added a comment -

            Thank you for the helpful review Pim Schellart [X]. I adopted your suggested changes, including taking advantage of utils::cppIndex for SpherePoint.__getitem__, which required an API simplification to cppIndex. I wanted to make that change anyway and I appreciate the impetus to do it.

            Show
            rowen Russell Owen added a comment - Thank you for the helpful review Pim Schellart [X] . I adopted your suggested changes, including taking advantage of utils::cppIndex for SpherePoint.__getitem__ , which required an API simplification to cppIndex . I wanted to make that change anyway and I appreciate the impetus to do it.

              People

              • Assignee:
                rowen Russell Owen
                Reporter:
                pschella Pim Schellart [X] (Inactive)
                Watchers:
                Krzysztof Findeisen, Pim Schellart [X] (Inactive), Russell Owen
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel