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

Edit testdata_cfht to pass obs_cfht unit tests

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: obs_cfht
    • Labels:
      None

      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.

        Attachments

          Issue Links

            Activity

            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            I created a pull request at testdata_cfht

            Show
            hchiang2 Hsin-Fang Chiang added a comment - I created a pull request at testdata_cfht
            Hide
            wmwood-vasey Michael Wood-Vasey 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
            wmwood-vasey Michael Wood-Vasey 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
            hchiang2 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
            hchiang2 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
            hchiang2 Hsin-Fang Chiang added a comment -

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

            Show
            hchiang2 Hsin-Fang Chiang added a comment - By any chance is there a problem with git-lfs installation?
            Hide
            wmwood-vasey Michael Wood-Vasey 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
            wmwood-vasey Michael Wood-Vasey 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
            hchiang2 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
            hchiang2 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
            wmwood-vasey Michael Wood-Vasey 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
            wmwood-vasey Michael Wood-Vasey 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
            hchiang2 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
            hchiang2 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
            hchiang2 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
            hchiang2 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
            hchiang2 Hsin-Fang Chiang added a comment -

            All checks on the PR have passed.

            Show
            hchiang2 Hsin-Fang Chiang added a comment - All checks on the PR have passed.
            Hide
            wmwood-vasey Michael Wood-Vasey 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
            wmwood-vasey Michael Wood-Vasey 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
            hchiang2 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
            hchiang2 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
            hchiang2 Hsin-Fang Chiang added a comment -

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

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

              People

              • Assignee:
                hchiang2 Hsin-Fang Chiang
                Reporter:
                hchiang2 Hsin-Fang Chiang
                Reviewers:
                Michael Wood-Vasey
                Watchers:
                Hsin-Fang Chiang, John Swinbank, Michael Wood-Vasey
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel