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

Incorporate l1dbproto into AssociationTask

    Details

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

      Description

      Take the existing AssociationTask functionality and get it running using l1dbproto (rather than SQLite) as a back-end.

      (This can run on a standalone system; no need to deploy at LDF.)

        Attachments

          Activity

          Hide
          cmorrison Chris Morrison added a comment -

          Hi you two,

          I've put together a wrapper for the prototype L1db that allows it to interface with source association. Yusra AlSayyad, if you could take a look at the ap_assocation code (and the l1db code if you like) that would be great.

          Andy Salnikov I've made a few changes to how l1db converts to afw objects and added dependencies for the afw.schema I have in ap_assocaition. I want to make sure that the changes don't adversely effect any of the use cases you have for l1dbproto. I've tested your scripts (create_l1_schema and ap_proto) with the l1db-sqlite.py config file and things seem to run to completion and produce what looks like a reasonable database at the end. Take a look and let me know if anything needs tweaking.

          Thanks!

          Show
          cmorrison Chris Morrison added a comment - Hi you two, I've put together a wrapper for the prototype L1db that allows it to interface with source association. Yusra AlSayyad , if you could take a look at the ap_assocation code (and the l1db code if you like) that would be great. Andy Salnikov I've made a few changes to how l1db converts to afw objects and added dependencies for the afw.schema I have in ap_assocaition. I want to make sure that the changes don't adversely effect any of the use cases you have for l1dbproto. I've tested your scripts (create_l1_schema and ap_proto) with the l1db-sqlite.py config file and things seem to run to completion and produce what looks like a reasonable database at the end. Take a look and let me know if anything needs tweaking. Thanks!
          Hide
          cmorrison Chris Morrison added a comment -

          Sorry, in my excitement to finish the ticket I forgot to create pull requests. They are up now.

          Show
          cmorrison Chris Morrison added a comment - Sorry, in my excitement to finish the ticket I forgot to create pull requests. They are up now.
          Hide
          salnikov Andy Salnikov added a comment - - edited

          Hi Chris, 

          sorry for delay, swamped with reviews and stuff.

          One thing that worries me more than others is that you introduced circular dependency between ap_association and l1dbproto. ap_association needs to depend on l1dbproto of course, but opposite dependency should be avoided. I see that you imported lsst.ap.association into ap_proto and create_l1_schema. I guess you will be needing a script to create schema for ap_association, and I think the easiest is to just copy create_l1_schema from l1dbproto and modify it for your needs. ap_proto does not really need to use exactly the same schema as ap_association so we can probably keep whatever schema it was using before (and I can update that later to match ap_association if needed).

          Another thing that I want to check before approving those changes is to verify that degrees to radians conversion works as expected. There may be other locations in ap_proto that depend on units of ra/decl columns, the code may still "work" with different units but it may produce wrong results. Give me day or two to try to run stuff from your branch and see how it behaves.

          [Also, if you are ready for merging can you open pull request on github so that we can comment on the code? -- taking this out, took me too long to write my comment]

          Cheers,
          Andy

          Show
          salnikov Andy Salnikov added a comment - - edited Hi Chris,  sorry for delay, swamped with reviews and stuff. One thing that worries me more than others is that you introduced circular dependency between ap_association and l1dbproto. ap_association needs to depend on l1dbproto of course, but opposite dependency should be avoided. I see that you imported lsst.ap.association into ap_proto and create_l1_schema. I guess you will be needing a script to create schema for ap_association, and I think the easiest is to just copy create_l1_schema from l1dbproto and modify it for your needs. ap_proto does not really need to use exactly the same schema as ap_association so we can probably keep whatever schema it was using before (and I can update that later to match ap_association if needed). Another thing that I want to check before approving those changes is to verify that degrees to radians conversion works as expected. There may be other locations in ap_proto that depend on units of ra/decl columns, the code may still "work" with different units but it may produce wrong results. Give me day or two to try to run stuff from your branch and see how it behaves. [Also, if you are ready for merging can you open pull request on github so that we can comment on the code? -- taking this out, took me too long to write my comment] Cheers, Andy
          Hide
          cmorrison Chris Morrison added a comment -

          Hey Andy,

          Thanks for looking at this. I've removed the ap_association dependencies as asked. I mostly had them in there so I could test the association schema in the context of your scripts.

          Show
          cmorrison Chris Morrison added a comment - Hey Andy, Thanks for looking at this. I've removed the ap_association dependencies as asked. I mostly had them in there so I could test the association schema in the context of your scripts.
          Hide
          salnikov Andy Salnikov added a comment -

          Thanks, Chris! I also pushed couple of small commits  ap_proto had deg<>rad conversion that needed a fix after you switched to Angle in afw table. Also in you last commit imports did not work for me for some reason., had to fix those too. I'll go through PR review later today after all meetings. 

           

          Show
          salnikov Andy Salnikov added a comment - Thanks, Chris! I also pushed couple of small commits  ap_proto had deg< >rad conversion that needed a fix after you switched to Angle in afw table. Also in you last commit imports did not work for me for some reason., had to fix those too. I'll go through PR review later today after all meetings.   
          Hide
          salnikov Andy Salnikov added a comment -

          Chris Morrison, sorry for delay, I have reviewed both l1dbproto and (very superficially) ap_association changes, check my comments on PR. I think one small unit-related improvement would be useful in l1dbproto. otherwise it all looks OK. I'm removing myself from reviewers, Yusra AlSayyad should mark it as reviewed when done reviewing.

          Show
          salnikov Andy Salnikov added a comment - Chris Morrison , sorry for delay, I have reviewed both l1dbproto and (very superficially) ap_association changes, check my comments on PR. I think one small unit-related improvement would be useful in l1dbproto. otherwise it all looks OK. I'm removing myself from reviewers, Yusra AlSayyad should mark it as reviewed when done reviewing.
          Hide
          cmorrison Chris Morrison added a comment -

          No delay at all Andy Salnikov. Thanks for having a look and glad that most of the changes were reasonable. I'm taking a look through your comments now.

          Show
          cmorrison Chris Morrison added a comment - No delay at all Andy Salnikov . Thanks for having a look and glad that most of the changes were reasonable. I'm taking a look through your comments now.
          Hide
          yusra Yusra AlSayyad added a comment -

          See comments posted in github

          Show
          yusra Yusra AlSayyad added a comment - See comments posted in github

            People

            • Assignee:
              cmorrison Chris Morrison
              Reporter:
              swinbank John Swinbank
              Reviewers:
              Yusra AlSayyad
              Watchers:
              Andy Salnikov, Chris Morrison, Eric Bellm, John Swinbank, Yusra AlSayyad
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Summary Panel