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

Rewrite updateRefCentroids and updateSourceCoords to convert all positions at once

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Story Points:
      2
    • Epic Link:
    • Sprint:
      AP S18-2
    • Team:
      Alert Production

      Description

      The current updateRefCentroids and updateSourceCoords functions are pure Python and compute one position at a time. SkyWcs performs much better if it can convert a batch of values at once.

      I could rewrite the Python code to extract all the positions, convert them, then loop through the catalog again. But I worry this will be needlessly slow (the looping is already slow, and this will only make it worse), so I propose to rewrite the functions in C++ instead.

        Attachments

          Issue Links

          is blocked by

          Improvement - An improvement or enhancement to an existing feature or task. DM-10765 Replace existing WCS classes with SkyWcs

          • Done

            Activity

            Ascending order - Click to sort in descending order
            rowen Russell Owen created issue -
            rowen Russell Owen made changes -
            Field Original Value New Value
            Epic Link DM-9679 [ 30784 ]
            rowen Russell Owen made changes -
            Link This issue is blocked by DM-10765 [ DM-10765 ]
            Hide
            Permalink
            rowen Russell Owen added a comment -

            Please have a look. I used overloads to factor out common code, but it's messier than I'd like. Suggestions for cleanup would be most welcome. Issues:

            • It will only work for SortedCatalogT catalogs, which is fine for the current use case, but disappointing. Using Catalog as a template parameter didn't work because it is ambiguous and my compiler tried to use that template for vectors of shared pointers to records.
            • The aggregate keys don't seem to have a Value attribute so I have Value and Key as separate template parameters. Ick.

            I would prefer to have something like public versions of getValue and setValue that worked for all catalog types, key types and match lists (allowing the two functions this ticket is about to be written in Python and be fairly efficient). But the number of instantiations required for that seems prohibitive.

            The pull request (which is against DM-10765) is here: https://github.com/lsst/afw/pull/313

            Show
            rowen Russell Owen added a comment - Please have a look. I used overloads to factor out common code, but it's messier than I'd like. Suggestions for cleanup would be most welcome. Issues: It will only work for SortedCatalogT catalogs, which is fine for the current use case, but disappointing. Using Catalog as a template parameter didn't work because it is ambiguous and my compiler tried to use that template for vectors of shared pointers to records. The aggregate keys don't seem to have a Value attribute so I have Value and Key as separate template parameters. Ick. I would prefer to have something like public versions of getValue and setValue that worked for all catalog types, key types and match lists (allowing the two functions this ticket is about to be written in Python and be fairly efficient). But the number of instantiations required for that seems prohibitive. The pull request (which is against DM-10765 ) is here: https://github.com/lsst/afw/pull/313
            rowen Russell Owen made changes -
            Reviewers Jim Bosch [ jbosch ]
            Status To Do [ 10001 ] In Review [ 10004 ]
            Hide
            Permalink
            jbosch Jim Bosch added a comment -

            Review complete; more comments on the PR (but I'll reply to your comments above here).

             - I think you can make this work for more types of catalogs by using CatalogT<Record> instead of SortedCatalogT<Record>. That should also be non-ambiguous.

             - I very much agree that it'd be good to add Value typedefs to the FunctorKeys.  I'd be supportive of either doing that on this ticket or making that a separate ticket.

             - I think it is hard to do this generically and make that usable from Python.  The more Pythonic thing to do might be to find a way to make ColumnView return Point or Coord columns in a form that's compatible with SkyWcs/Transform, and have a faster, easier way to turn a list of Records or a list of pairs of Records into a Catalog.  That's all well beyond the scope of this ticket, of course.

            Show
            jbosch Jim Bosch added a comment - Review complete; more comments on the PR (but I'll reply to your comments above here).  - I think you can make this work for more types of catalogs by using CatalogT<Record> instead of SortedCatalogT<Record> . That should also be non-ambiguous.  - I very much agree that it'd be good to add Value typedefs to the FunctorKeys .  I'd be supportive of either doing that on this ticket or making that a separate ticket.  - I think it is hard to do this generically and make that usable from Python.  The more Pythonic thing to do might be to find a way to make ColumnView return Point or Coord columns in a form that's compatible with SkyWcs/ Transform , and have a faster, easier way to turn a list of Records  or a list of pairs of Records  into a Catalog.  That's all well beyond the scope of this ticket, of course.
            jbosch Jim Bosch made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            Permalink
            rowen Russell Owen added a comment -

            Thanks for the helpful review. I implemented all of your suggestions except making the public functions explicit overloads that delegate (which was more than I wanted to tackle at this time).

            Could you please have another quick look, especially since I added a Value typedef to FunctorKey and its two parent classes and I want to be sure you are comfortable with the way I did it.

            Show
            rowen Russell Owen added a comment - Thanks for the helpful review. I implemented all of your suggestions except making the public functions explicit overloads that delegate (which was more than I wanted to tackle at this time). Could you please have another quick look, especially since I added a Value typedef to FunctorKey and its two parent classes and I want to be sure you are comfortable with the way I did it.
            rowen Russell Owen made changes -
            Status Reviewed [ 10101 ] In Review [ 10004 ]
            Hide
            Permalink
            jbosch Jim Bosch added a comment -

            Looks great!

            Show
            jbosch Jim Bosch added a comment - Looks great!
            jbosch Jim Bosch made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            Permalink
            rowen Russell Owen added a comment -

            Merged to DM-10765

            Show
            rowen Russell Owen added a comment - Merged to DM-10765
            rowen Russell Owen made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

              People

              Assignee:
              rowen Russell Owen
              Reporter:
              rowen Russell Owen
              Reviewers:
              Jim Bosch
              Watchers:
              Jim Bosch, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: