# Edit testdata_cfht to pass obs_cfht unit tests

XMLWordPrintable

#### Details

• Type: Bug
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
1
• Team:
Data Facility

#### Description

This ticket covers the first half of the issues in DM-2917.

testdata_cfht was left unedited while some past changes in obs_cfht MegacamMapper required coordinated changes. The goal of this ticket is to simply pass the unit tests currently in obs_cfht.

#### Activity

Hide
Hsin-Fang Chiang added a comment -

I created a pull request at testdata_cfht

Show
Hsin-Fang Chiang added a comment - I created a pull request at testdata_cfht
Hide
Michael Wood-Vasey [X] (Inactive) added a comment -

I understand the scope of this ticket to be

Fix testdata_cfht so that the tests in obs_cfht pass.

Do you believe it might be necessary to change obs_cfht?

Show
Michael Wood-Vasey [X] (Inactive) added a comment - I understand the scope of this ticket to be Fix testdata_cfht so that the tests in obs_cfht pass. Do you believe it might be necessary to change obs_cfht?
Hide
Hsin-Fang Chiang added a comment -

I didn't change the unit test in obs_cfht, and the branch tickets/DM-4711 of testdata_cfht should pass with the current master of obs_cfht (59e204f). (Or at least it does on my machine).

Show
Hsin-Fang Chiang added a comment - I didn't change the unit test in obs_cfht , and the branch tickets/ DM-4711 of testdata_cfht should pass with the current master of obs_cfht (59e204f). (Or at least it does on my machine).
Hide
Hsin-Fang Chiang added a comment -

By any chance is there a problem with git-lfs installation?

Show
Hsin-Fang Chiang added a comment - By any chance is there a problem with git-lfs installation?
Hide
Michael Wood-Vasey [X] (Inactive) added a comment -

Problem I was having was related to a failed parentSearch wiping out the filename. This has been filed as bug

https://jira.lsstcorp.org/browse/DM-4732

I don't clearly understand why this doesn't affect Hsin-Fang Chiang in her tests,

The proposed change represented by the pull request is clearly an improvement and should be adopted and merged into master as is.

Show
Michael Wood-Vasey [X] (Inactive) added a comment - Problem I was having was related to a failed parentSearch wiping out the filename. This has been filed as bug https://jira.lsstcorp.org/browse/DM-4732 I don't clearly understand why this doesn't affect Hsin-Fang Chiang in her tests, The proposed change represented by the pull request is clearly an improvement and should be adopted and merged into master as is.
Hide
Hsin-Fang Chiang added a comment -

I was using stack b1817 (11-Dec-2015) on my machine, and I am able to reproduce the errors you saw on lsst-dev with a newer stack. (p.s. it passes with b1817 on lsst-dev too)

As you suggested, I merged the changes in testdata_cfht, and leave the butler issue to DM-4732.

Show
Hsin-Fang Chiang added a comment - I was using stack b1817 (11-Dec-2015) on my machine, and I am able to reproduce the errors you saw on lsst-dev with a newer stack. (p.s. it passes with b1817 on lsst-dev too) As you suggested, I merged the changes in testdata_cfht , and leave the butler issue to DM-4732 .
Hide
Michael Wood-Vasey [X] (Inactive) added a comment -

Hsin-Fang Chiang
Do you plan to merge tickets/DM-4711 in lsst/lsstsw.git in to master

to include obs_cfht, testdata_cfht, and obs_decam in the lsstsw/etc/repos.yaml file?

Show
Michael Wood-Vasey [X] (Inactive) added a comment - Hsin-Fang Chiang Do you plan to merge tickets/ DM-4711 in lsst/lsstsw.git in to master to include obs_cfht , testdata_cfht , and obs_decam in the lsstsw/etc/repos.yaml file?
Hide
Hsin-Fang Chiang added a comment -

I forgot about that. The previous branch wasn't quite correct and I just pushed a corrected version in.

I'm reading RFC-75 and just created a PR at lsstsw

It seems a bit weird to do this merge under this ticket though.

Show
Hsin-Fang Chiang added a comment - I forgot about that. The previous branch wasn't quite correct and I just pushed a corrected version in. I'm reading RFC-75 and just created a PR at lsstsw It seems a bit weird to do this merge under this ticket though.
Hide
Hsin-Fang Chiang added a comment - - edited

I don't understand the CI enough to self-merge yet. I vaguely remember some concerns about the large size of testdata_cfht possibly getting pulled to CI, but can't find the conversations on HipChat now.

Tim Jenness is it safe to add testdata_cfht to lsstsw/etc/repos.yaml?

Show
Hsin-Fang Chiang added a comment - - edited I don't understand the CI enough to self-merge yet. I vaguely remember some concerns about the large size of testdata_cfht possibly getting pulled to CI, but can't find the conversations on HipChat now. Tim Jenness is it safe to add testdata_cfht to lsstsw/etc/repos.yaml ?
Hide
Hsin-Fang Chiang added a comment -

All checks on the PR have passed.

Show
Hsin-Fang Chiang added a comment - All checks on the PR have passed.
Hide
Michael Wood-Vasey [X] (Inactive) added a comment - - edited

testdata_cfht and obs_cfht (or obs_decam, testdata_decam) won't get pulled for the default Jenkins build of lsst_distrib or lsst_sims, which are the standard products that are built by the hourly build.

Eventually, there's a larger question of how to include testdata_* in adding to CI, but adding this to lsstsw/etc/repos.yaml doesn't immediately change anything. However, it does make it easier to use the standard eups distrib or lsstsw builds of these additional packages.

I would bet that most changes to lsstsw and particularly for etc/repos.yaml will be associated with some ticket that was mostly about working on some other product.

I give the (thumbsup) to merge the additional lines in the repo.yaml file to master.

Show
Michael Wood-Vasey [X] (Inactive) added a comment - - edited testdata_cfht and obs_cfht (or obs_decam , testdata_decam ) won't get pulled for the default Jenkins build of lsst_distrib or lsst_sims , which are the standard products that are built by the hourly build. Eventually, there's a larger question of how to include testdata_* in adding to CI, but adding this to lsstsw/etc/repos.yaml doesn't immediately change anything. However, it does make it easier to use the standard eups distrib or lsstsw builds of these additional packages. I would bet that most changes to lsstsw and particularly for etc/repos.yaml will be associated with some ticket that was mostly about working on some other product. I give the (thumbsup) to merge the additional lines in the repo.yaml file to master.
Hide
Hsin-Fang Chiang added a comment -

Found the conversations in HipChat (Dec 22, 2015, CFHT reprocessing room).

It seems alright to merge, and the concern is about building it through Jenkins. So I'll go ahead to merge it.

Show
Hsin-Fang Chiang added a comment - Found the conversations in HipChat (Dec 22, 2015, CFHT reprocessing room). It seems alright to merge, and the concern is about building it through Jenkins. So I'll go ahead to merge it.
Hide
Hsin-Fang Chiang added a comment -

I replied before seeing you reply. Thanks for confirming Michael Wood-Vasey [X]! I've just merged the changes in lsstsw.

Show
Hsin-Fang Chiang added a comment - I replied before seeing you reply. Thanks for confirming Michael Wood-Vasey [X] ! I've just merged the changes in lsstsw .

#### People

Assignee:
Hsin-Fang Chiang
Reporter:
Hsin-Fang Chiang
Reviewers:
Michael Wood-Vasey [X] (Inactive)
Watchers:
Hsin-Fang Chiang, John Swinbank, Michael Wood-Vasey [X] (Inactive)