# Add UC Davis camera support to obs_lsst

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
2
• Sprint:
Arch 2019-01-28
• Team:
Architecture

#### 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.

#### Activity

No builds found.
John Swinbank created issue -
Field Original Value New Value
Epic Link DM-16678 [ 235238 ]
 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
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
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.
 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
John Swinbank added a comment -

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

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

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
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
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
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
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
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
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
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.
 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 ]
 Assignee Tim Jenness [ tjenness ]
 Team Data Release Production [ 10301 ] Architecture [ 10304 ]
 Epic Link DM-16678 [ 235238 ]
Hide
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
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.
 Sprint Arch 2019-01-28 [ 853 ]
Hide
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
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.
 Reviewers Merlin Fisher-Levine [ mfisherlevine ] Status To Do [ 10001 ] In Review [ 10004 ]
 Reviewers Merlin Fisher-Levine [ mfisherlevine ] Robert Lupton [ rhl ]
Hide
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
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
Robert Lupton added a comment -

I can do the review

Show
Robert Lupton added a comment - I can do the review
Hide
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
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
 Status In Review [ 10004 ] Reviewed [ 10101 ]
Hide
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
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.
 Resolution Done [ 10000 ] Status Reviewed [ 10101 ] Done [ 10002 ]
 Epic Link DM-16576 [ 234914 ]

#### People

Assignee:
Tim Jenness
Reporter:
John Swinbank
Reviewers:
Robert Lupton
Watchers:
Craig Lage, John Swinbank, Merlin Fisher-Levine, Robert Lupton, Simon Krughoff, Tim Jenness