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

Implement simple DIAObject

    Details

      Description

      Implement the design as planned in DM-11038.

        Attachments

          Activity

          Hide
          cmorrison Chris Morrison added a comment -

          Started work on the "guts" of the DIAObject class methods now that DM-11038 is in review.

          Show
          cmorrison Chris Morrison added a comment - Started work on the "guts" of the DIAObject class methods now that DM-11038 is in review.
          Hide
          cmorrison Chris Morrison added a comment -

          Finished first pass on MVP/S DIAObject and unittest. Current code can be see at https://github.com/lsst-dm/ap_association/tree/tickets/DM-11039

          Unittesting and code clean up for review should happen tomorrow.

          Show
          cmorrison Chris Morrison added a comment - Finished first pass on MVP/S DIAObject and unittest. Current code can be see at https://github.com/lsst-dm/ap_association/tree/tickets/DM-11039 Unittesting and code clean up for review should happen tomorrow.
          Hide
          cmorrison Chris Morrison added a comment -

          Finished first pass at DIAObject implementation for MVP/S. Code to review is dia_object.py and test_dia_object.py on the following ticket branch. https://github.com/lsst-dm/ap_association/tree/tickets/DM-11039

          Show
          cmorrison Chris Morrison added a comment - Finished first pass at DIAObject implementation for MVP/S. Code to review is dia_object.py and test_dia_object.py on the following ticket branch. https://github.com/lsst-dm/ap_association/tree/tickets/DM-11039
          Hide
          ctslater Colin Slater added a comment -

          Review is posted on github. My main issue is that this does not seem to be the right place to define how, for example, coordinates get aggregated into a mean coordinate. These aggregation functions deserve some significant attention on their own, and shouldn't be buried inside DiaObject. I think it really belongs in afw with the rest of our coordinate and angle manipulation functions.

          The other broad topic is that I can't quite tell if the dia_object_record is actually a purely-internal implementation detail, or if it's meant to be publicly exposed. The code seems to waffle on this; it's stored with a leading underscore, but then it's also a parameter to the constructor? This breaks encapsulation and exposes a lot of messiness to the user.

          Show
          ctslater Colin Slater added a comment - Review is posted on github. My main issue is that this does not seem to be the right place to define how, for example, coordinates get aggregated into a mean coordinate. These aggregation functions deserve some significant attention on their own, and shouldn't be buried inside DiaObject . I think it really belongs in afw with the rest of our coordinate and angle manipulation functions. The other broad topic is that I can't quite tell if the dia_object_record is actually a purely-internal implementation detail, or if it's meant to be publicly exposed. The code seems to waffle on this; it's stored with a leading underscore, but then it's also a parameter to the constructor? This breaks encapsulation and exposes a lot of messiness to the user.
          Hide
          ctslater Colin Slater added a comment -

          Handful of cleanups still need to be addressed on the PR, otherwise looks good.

          Show
          ctslater Colin Slater added a comment - Handful of cleanups still need to be addressed on the PR, otherwise looks good.
          Hide
          cmorrison Chris Morrison added a comment -

          pushed to master on lsst-dm/ap_association

          Show
          cmorrison Chris Morrison added a comment - pushed to master on lsst-dm/ap_association

            People

            • Assignee:
              cmorrison Chris Morrison
              Reporter:
              cmorrison Chris Morrison
              Reviewers:
              Colin Slater
              Watchers:
              Chris Morrison, Colin Slater, Eric Bellm, Simon Krughoff
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel