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

Wrap leftover tests in afw with pybind11

    XMLWordPrintable

    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

          blocks

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-7057 Complete afw port to pybind11

          • Done
          is blocked by

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-6297 Wrap afw::detection with pybind11

          • Done

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-8415 Finish wrapping of pickling with pybind11 in afw

          • Done

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-8652 Wrap afw ds9 test with pybind11

          • Done

          Story - Created by Jira Software - do not edit or delete. Issue type for a user story. DM-8674 Wrap testSourceTable with pybind11

          • Done

            Activity

            Ascending order - Click to sort in descending order
            No builds found.
            pschella Pim Schellart [X] (Inactive) created issue -
            pschella Pim Schellart [X] (Inactive) made changes -
            Field Original Value New Value
            Epic Link DM-7717 [ 26925 ]
            pschella Pim Schellart [X] (Inactive) made changes -
            Link This issue blocks DM-7057 [ DM-7057 ]
            krzys Krzysztof Findeisen made changes -
            Assignee Krzysztof Findeisen [ krzys ]
            krzys Krzysztof Findeisen made changes -
            Component/s afw [ 10714 ]
            Team Alert Production [ 10300 ]
            Labels Pybind11 SciencePipelines
            krzys Krzysztof Findeisen made changes -
            Epic Link DM-7717 [ 26925 ] DM-8450 [ 28066 ]
            krzys Krzysztof Findeisen made changes -
            Sprint Alert Production S17 - 12 [ 305 ]
            krzys Krzysztof Findeisen made changes -
            Description Wrap any afw tests in {{tests/test.txt}} that are not covered Wrap any afw tests in {{tests/test.txt}} that are not covered.

            This includes:
            # testExecutables.py
            # testLeastSquares.py
            # testTableIO.py
            # testVisitInfo.py
            # testWeather.py
            krzys Krzysztof Findeisen made changes -
            Link This issue is blocked by DM-8415 [ DM-8415 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue is blocked by DM-6297 [ DM-6297 ]
            Hide
            Permalink
            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.
            krzys Krzysztof Findeisen made changes -
            Link This issue is blocked by DM-8652 [ DM-8652 ]
            krzys Krzysztof Findeisen made changes -
            Link This issue is blocked by DM-8674 [ DM-8674 ]
            krzys Krzysztof Findeisen made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            krzys Krzysztof Findeisen made changes -
            Description Wrap any afw tests in {{tests/test.txt}} that are not covered.

            This includes:
            # testExecutables.py
            # testLeastSquares.py
            # testTableIO.py
            # testVisitInfo.py
            # testWeather.py             		Wrap any afw tests in {{tests/test.txt}} that are not covered.

            This includes:
            # testExecutables.py
            # -testLeastSquares.py-
            # -testTableIO.py-
            # -testVisitInfo.py-
            # -testWeather.py-
            Hide
            Permalink
            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 .
            rowen Russell Owen made changes -
            Comment [ Also I find that if I restore the original tests/SConscript file (thus enabling C++ tests as well) that the build fails on my Mac with errors such as
            {code}
            Undefined symbols for architecture x86_64:
              "_PyBytes_AsStringAndSize", referenced from:
                  pybind11::str::operator std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >() const in testTableArchivesLib.o
                  pybind11::detail::type_caster<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, void>::load(pybind11::handle, bool) in testTableArchivesLib.o
              "_PyCFunction_GetSelf", referenced from:
                  pybind11::cpp_function::initialize_generic(pybind11::detail::function_record*, char const*, std::type_info const* const*, unsigned long) in testTableArchivesLib.o
            {code}

            I have no idea what's going on, but it's worrisome since we have all tests enabled in {{test.txt}}. ]
            Hide
            Permalink
            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.
            rowen Russell Owen made changes -
            Assignee Krzysztof Findeisen [ krzys ] Russell Owen [ rowen ]
            rowen Russell Owen made changes -
            Story Points 4 1
            Hide
            Permalink
            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
            Permalink
            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).
            rowen Russell Owen made changes -
            Story Points 1 4
            Hide
            Permalink
            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.
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status In Progress [ 3 ] Done [ 10002 ]

              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:

                  Jenkins

                  No builds found.