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

Create Gen3 versions of ap_verify datasets

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Story Points:
      4
    • Sprint:
      AP S20-5 (April), AP S20-6 (May)
    • Team:
      Alert Production
    • Urgent?:
      No

      Description

      Produce a Gen3 version of existing ap_verify datasets, including the dataset compiled in DM-24259.

        Attachments

          Issue Links

            Activity

            Hide
            krzys Krzysztof Findeisen added a comment -

            Tim Jenness, obs_test has a couple of images from old lsstSim, per the readme. I am not aware of any requirement that data be from a real instrument.

            Show
            krzys Krzysztof Findeisen added a comment - Tim Jenness , obs_test has a couple of images from old lsstSim, per the readme . I am not aware of any requirement that data be from a real instrument.
            Hide
            tjenness Tim Jenness added a comment -

            Yes, I know that obs_test has a couple of old sims images. What I'm confused by is that ap_verify_testdata depends on obs_test.

            Show
            tjenness Tim Jenness added a comment - Yes, I know that obs_test has a couple of old sims images. What I'm confused by is that ap_verify_testdata depends on obs_test.
            Hide
            jbosch Jim Bosch added a comment -

            All looks good; I didn't have much to say, but there are a few comments on the _templates PR.

            I did not look at the collection structure in the converted repos - I think the code you're using to create them probably guarantees that they're close enough to what we're envisioning that it's not worth holding anything up right now. As a heads up I think we'll want to have another pass at defining those conventions more clearly in the future (RFC-663 is clearly proving to be at least insufficient, and in some cases maybe made poor choices), so these probably shouldn't be considered set in stone (and hopefully the code you delegate to will continue to make changes there propagate through to here automatically - though that may not be the case for code that uses the datasets created here.

            It's a bit unfortunate that daf_butler and ap_verify use "dataset" to mean very specific and quite different things, but I'm sure this is now too baked in to both to change, and it's not completely surprising given that the meaning of the word outside our codebase is somewhat ambiguous.

            Show
            jbosch Jim Bosch added a comment - All looks good; I didn't have much to say, but there are a few comments on the _templates PR. I did not look at the collection structure in the converted repos - I think the code you're using to create them probably guarantees that they're close enough to what we're envisioning that it's not worth holding anything up right now. As a heads up I think we'll want to have another pass at defining those conventions more clearly in the future ( RFC-663 is clearly proving to be at least insufficient, and in some cases maybe made poor choices), so these probably shouldn't be considered set in stone (and hopefully the code you delegate to will continue to make changes there propagate through to here automatically - though that may not be the case for code that uses the datasets created here. It's a bit unfortunate that daf_butler and ap_verify use "dataset" to mean very specific and quite different things, but I'm sure this is now too baked in to both to change, and it's not completely surprising given that the meaning of the word outside our codebase is somewhat ambiguous.
            Hide
            mrawls Meredith Rawls added a comment -

            Re. ap_verify_testdata depending on obs_test, I believe we did that because we needed some manner of "real" image data to run tests on and wanted to keep the testdata package from being any larger than it needed to be. Since perfectly acceptable test images already exist, and we didn't want to tie ap_verify_testdata to any particular obs_ package, we went with "generic" image data in obs_test. This could be pretty easily circumvented by putting a copy of the data that's currently loaded from obs_test directly in the ap_verify_testdata repo.

            Show
            mrawls Meredith Rawls added a comment - Re. ap_verify_testdata depending on obs_test, I believe we did that because we needed some manner of "real" image data to run tests on and wanted to keep the testdata package from being any larger than it needed to be. Since perfectly acceptable test images already exist, and we didn't want to tie ap_verify_testdata to any particular obs_ package, we went with "generic" image data in obs_test. This could be pretty easily circumvented by putting a copy of the data that's currently loaded from obs_test directly in the ap_verify_testdata repo.
            Hide
            krzys Krzysztof Findeisen added a comment -

            Thanks for the review! For the record, the current reference for ap_verify datasets' collection structure is the discussion in DM-21915 (I just realized that I should also document that in ap_verify_dataset_templates, since its documentation is the "official" spec for what an ap_verify dataset should look like).

            As for the term "dataset", yes, it's been a problem since the Gen 2 days. I could swear we had a Slack discussion about it a few years back, but now I can't find it.

            Show
            krzys Krzysztof Findeisen added a comment - Thanks for the review! For the record, the current reference for ap_verify datasets' collection structure is the discussion in DM-21915 (I just realized that I should also document that in ap_verify_dataset_templates , since its documentation is the "official" spec for what an ap_verify dataset should look like). As for the term "dataset", yes, it's been a problem since the Gen 2 days. I could swear we had a Slack discussion about it a few years back, but now I can't find it.

              People

              • Assignee:
                krzys Krzysztof Findeisen
                Reporter:
                swinbank John Swinbank
                Reviewers:
                Jim Bosch
                Watchers:
                Eric Bellm, Jim Bosch, John Swinbank, Krzysztof Findeisen, Meredith Rawls, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: