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

Add UC Davis camera support to obs_lsst

    XMLWordPrintable

    Details

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

      Description

      In obs_lsst #43 Craig Lage has added support for the UC Davis camera to obs_lsst.

      We need to shepherd this through adoption into the DM codebase.
       

        Attachments

          Activity

          No builds found.
          swinbank John Swinbank created issue -
          swinbank John Swinbank made changes -
          Field Original Value New Value
          Epic Link DM-16678 [ 235238 ]
          mfisherlevine Merlin Fisher-Levine made changes -
          Description In [obs_lsst #43|https://github.com/lsst/obs_lsst/pull/43] [~cslage] has added support for the UC Davis camera to obs_lsst.

          Per assorted discussions, this seems like a good thing to have.

          We should have somebody do a review of the code (I believe [~mfisherlevine] has already done much of this...) and to make sure that it's covered by the test suite (which I haven't properly investigated, but it looks as though adding some UCDCam data to https://github.com/lsst-dm/testdata_obs_lsst would be a good start).
          In [obs_lsst #43|https://github.com/lsst/obs_lsst/pull/43] [~cslage] has added support for the UC Davis camera to obs_lsst.

          Per assorted discussions, this seems like a good thing to have.

          We should have somebody do a review of the code (I believe [~mfisherlevine] has already done much of this...) and to make sure that it's covered by the test suite (which I haven't properly investigated, but it looks as though adding some UCDCam data to [https://github.com/lsst-dm/testdata_obs_lsst] would be a good start).

          Plan of action
           * [~cslage] provides some naked headers to [~tjenness] (naked == without the data)
           * [~tjenness], like a saint, ports the header translation to `astro_metadata_translator`
           * Tests are written, both obs.base.test/pytest style ones, and `ci_lsst` ones using the data John refers to above.
           * [~cslage] tidies up the code to remove the crufty/vestigial sensors that are there due to copying ts8 code instead of AuxTel code
           * Merlin reviews the full changeset post-tidy-up
           * We merge
           * Everyone goes home happy.

           
          Hide
          swinbank John Swinbank added a comment - - edited

          Plan of action, courtesy of the fast-typing Merlin Fisher-Levine:

          • Craig Lage provides some naked headers to Tim Jenness (naked == without the data)
          • Tim Jenness, like a saint, ports the header translation to astro_metadata_translator
          • Tests are written, both obs.base.test/pytest style ones, and ci_lsst ones using the data John refers to above.
          • Craig Lage tidies up the code to remove the crufty/vestigial sensors that are there due to copying ts8 code instead of AuxTel code
          • Merlin Fisher-Levine reviews the full changeset post-tidy-up
          • We merge
          • Everyone goes home happy.

           

          Show
          swinbank John Swinbank added a comment - - edited Plan of action, courtesy of the fast-typing Merlin Fisher-Levine : Craig Lage provides some naked headers to Tim Jenness (naked == without the data) Tim Jenness , like a saint, ports the header translation to astro_metadata_translator Tests are written, both obs.base.test/pytest style ones, and ci_lsst ones using the data John refers to above. Craig Lage tidies up the code to remove the crufty/vestigial sensors that are there due to copying ts8 code instead of AuxTel code Merlin Fisher-Levine reviews the full changeset post-tidy-up We merge Everyone goes home happy.  
          swinbank John Swinbank made changes -
          Description In [obs_lsst #43|https://github.com/lsst/obs_lsst/pull/43] [~cslage] has added support for the UC Davis camera to obs_lsst.

          Per assorted discussions, this seems like a good thing to have.

          We should have somebody do a review of the code (I believe [~mfisherlevine] has already done much of this...) and to make sure that it's covered by the test suite (which I haven't properly investigated, but it looks as though adding some UCDCam data to [https://github.com/lsst-dm/testdata_obs_lsst] would be a good start).

          Plan of action
           * [~cslage] provides some naked headers to [~tjenness] (naked == without the data)
           * [~tjenness], like a saint, ports the header translation to `astro_metadata_translator`
           * Tests are written, both obs.base.test/pytest style ones, and `ci_lsst` ones using the data John refers to above.
           * [~cslage] tidies up the code to remove the crufty/vestigial sensors that are there due to copying ts8 code instead of AuxTel code
           * Merlin reviews the full changeset post-tidy-up
           * We merge
           * Everyone goes home happy.

           
          In [obs_lsst #43|https://github.com/lsst/obs_lsst/pull/43] [~cslage] has added support for the UC Davis camera to obs_lsst.

          We need to shepherd this through adoption into the DM codebase.
           
          Hide
          swinbank John Swinbank added a comment -

          Thanks, Merlin Fisher-Levine, for the above. A couple of points I'm not sure about:

          Tim Jenness, like a saint, ports the header translation to astro_metadata_translator

          Does this need to wait for the UCDcam implementation to switch from being based on TS8 to being based on AuxTel, or can Tim go ahead whenever he's ready?

          Tests are written, both obs.base.test/pytest style ones, and ci_lsst ones using the data John refers to above

          Following today's discussion on Slack, I'm actually a bit confused about this.

          I think that ci_lsst is deprecated and all test code now lives in obs_lsst, so bringing it up here is a red herring. However, this seems to be something that Simon Krughoff and Tim Jenness have opinions about, so perhaps they'd like to confirm (or contradict) my impression.

          Certainly, I understand that there are two types of test involved: one that involves using a script (runPipeline, I think) to “process the test data for the cameras from end-to-end” (which will require appropriate data to be provided), and one which follows the Python unittest pattern already established by e.g. tests/test_ts8.py and tests/test_auxTel.py.

          Craig Lage, are you in a position to put some tests together following the established patterns, or would you like some help from the DM side with this?

          Show
          swinbank John Swinbank added a comment - Thanks, Merlin Fisher-Levine , for the above. A couple of points I'm not sure about: Tim Jenness, like a saint, ports the header translation to astro_metadata_translator Does this need to wait for the UCDcam implementation to switch from being based on TS8 to being based on AuxTel, or can Tim go ahead whenever he's ready? Tests are written, both obs.base.test/pytest style ones, and ci_lsst ones using the data John refers to above Following today's discussion on Slack , I'm actually a bit confused about this. I think that ci_lsst is deprecated and all test code now lives in obs_lsst, so bringing it up here is a red herring. However, this seems to be something that Simon Krughoff and Tim Jenness have opinions about, so perhaps they'd like to confirm (or contradict) my impression. Certainly, I understand that there are two types of test involved: one that involves using a script ( runPipeline , I think) to “process the test data for the cameras from end-to-end” (which will require appropriate data to be provided), and one which follows the Python unittest pattern already established by e.g. tests/test_ts8.py and tests/test_auxTel.py . Craig Lage , are you in a position to put some tests together following the established patterns, or would you like some help from the DM side with this?
          Hide
          mfisherlevine Merlin Fisher-Levine added a comment -

          To answer the first question I think that's a question for Tim really. I imagine it won't make any real difference, other than the potential troubles of git conflicts with everyone working at the same time, but that's literally what it's supposed to make easy, so that should be fine I think.

           

          The other question, as you identify, sounds like it's best answered by Simon/Tim, and I don't care if ucdCam doesn't have `runPipeline` style tests, unit tests should be fine.

          Show
          mfisherlevine Merlin Fisher-Levine added a comment - To answer the first question I think that's a question for Tim really. I imagine it won't make any real difference, other than the potential troubles of git conflicts with everyone working at the same time, but that's literally what it's supposed to make easy, so that should be fine I think.   The other question, as you identify, sounds like it's best answered by Simon/Tim, and I don't care if ucdCam doesn't have `runPipeline` style tests, unit tests should be fine.
          Hide
          krughoff Simon Krughoff added a comment -

          I'm really sorry for causing a lot of confusion. I should not have attempted to deprecate ci_lsst. By moving things to obs_lsst I introduced a lot of setupOptional requirements which cause problems with lsstsw.

          I have a ticket to rectify this oversight on my part.

          Show
          krughoff Simon Krughoff added a comment - I'm really sorry for causing a lot of confusion. I should not have attempted to deprecate ci_lsst . By moving things to obs_lsst I introduced a lot of setupOptional requirements which cause problems with lsstsw . I have a ticket to rectify this oversight on my part.
          Hide
          swinbank John Swinbank added a comment -

          (Merlin)

          I don't care if ucdCam doesn't have `runPipeline` style tests, unit tests should be fine.

          Per OOB discussion, Merlin might not care, but I reckon we have to insist on them.

          (Simon)

          I'm really sorry for causing a lot of confusion...

          Don't be; thanks for attempting to bring order to the chaos of testing. Sounds like ci_lsst really is the right thing to do then.

          Show
          swinbank John Swinbank added a comment - (Merlin) I don't care if ucdCam doesn't have `runPipeline` style tests, unit tests should be fine. Per OOB discussion, Merlin might not care, but I reckon we have to insist on them. (Simon) I'm really sorry for causing a lot of confusion... Don't be; thanks for attempting to bring order to the chaos of testing. Sounds like ci_lsst really is the right thing to do then.
          tjenness Tim Jenness made changes -
          Watchers John Swinbank, Merlin Fisher-Levine, Simon Krughoff, Tim Jenness [ John Swinbank, Merlin Fisher-Levine, Simon Krughoff, Tim Jenness ] Craig Lage, John Swinbank, Merlin Fisher-Levine, Simon Krughoff, Tim Jenness [ Craig Lage, John Swinbank, Merlin Fisher-Levine, Simon Krughoff, Tim Jenness ]
          tjenness Tim Jenness made changes -
          Assignee Tim Jenness [ tjenness ]
          tjenness Tim Jenness made changes -
          Team Data Release Production [ 10301 ] Architecture [ 10304 ]
          tjenness Tim Jenness made changes -
          Epic Link DM-16678 [ 235238 ]
          Hide
          tjenness Tim Jenness added a comment -

          I've written a translator and modified the pull request to match the current code in obs_lsst. I've also updated the camera files to use the new approach where we have a separate pseudo-raft per CCD. Craig Lage is now updating the camera definitions to work properly because in his pull request he hard-coded the camera YAML file but in my branch the YAML file is generated during the build like all the other cameras.

          I will need help from Simon Krughoff at some point to ingest the example files into the butler gen2 repository inside the git repo.

          Show
          tjenness Tim Jenness added a comment - I've written a translator and modified the pull request to match the current code in obs_lsst. I've also updated the camera files to use the new approach where we have a separate pseudo-raft per CCD. Craig Lage is now updating the camera definitions to work properly because in his pull request he hard-coded the camera YAML file but in my branch the YAML file is generated during the build like all the other cameras. I will need help from Simon Krughoff at some point to ingest the example files into the butler gen2 repository inside the git repo.
          tjenness Tim Jenness made changes -
          Sprint Arch 2019-01-28 [ 853 ]
          Hide
          tjenness Tim Jenness added a comment -

          Merlin Fisher-Levine would you mind looking at this? We've added tests and sorted out the camera yaml file. I think I'd rather the addition of tests to ci_lsst be done on a different ticket (and preferably by someone else). Merging this to obs_lsst won't affect other people.

          Show
          tjenness Tim Jenness added a comment - Merlin Fisher-Levine would you mind looking at this? We've added tests and sorted out the camera yaml file. I think I'd rather the addition of tests to ci_lsst be done on a different ticket (and preferably by someone else). Merging this to obs_lsst won't affect other people.
          tjenness Tim Jenness made changes -
          Reviewers Merlin Fisher-Levine [ mfisherlevine ]
          Status To Do [ 10001 ] In Review [ 10004 ]
          tjenness Tim Jenness made changes -
          Reviewers Merlin Fisher-Levine [ mfisherlevine ] Robert Lupton [ rhl ]
          Hide
          tjenness Tim Jenness added a comment -

          Robert Lupton are you able to review this ticket? It should be relatively quick since you know the system.

          Show
          tjenness Tim Jenness added a comment - Robert Lupton are you able to review this ticket? It should be relatively quick since you know the system.
          Hide
          rhl Robert Lupton added a comment -

          I can do the review

          Show
          rhl Robert Lupton added a comment - I can do the review
          Hide
          rhl Robert Lupton added a comment -

          The code looks good. I'm still looking at adding some of the data to ci_lsst, but this need not prevent us from merging

          Show
          rhl Robert Lupton added a comment - The code looks good. I'm still looking at adding some of the data to ci_lsst, but this need not prevent us from merging
          rhl Robert Lupton made changes -
          Status In Review [ 10004 ] Reviewed [ 10101 ]
          Hide
          tjenness Tim Jenness added a comment -

          Merged. Thank you Robert Lupton for the review and thank you Craig Lage for starting this off with some code and helping out with my changes.

          Show
          tjenness Tim Jenness added a comment - Merged. Thank you Robert Lupton for the review and thank you Craig Lage for starting this off with some code and helping out with my changes.
          tjenness Tim Jenness made changes -
          Resolution Done [ 10000 ]
          Status Reviewed [ 10101 ] Done [ 10002 ]
          tjenness Tim Jenness made changes -
          Epic Link DM-16576 [ 234914 ]

            People

            Assignee:
            tjenness Tim Jenness
            Reporter:
            swinbank John Swinbank
            Reviewers:
            Robert Lupton
            Watchers:
            Craig Lage, John Swinbank, Merlin Fisher-Levine, Robert Lupton, Simon Krughoff, Tim Jenness
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                CI Builds

                No builds found.