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

Add brightObjectMasks to gen2convert

    Details

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

      Description

      assembleCoadd wants to apply the bright object masks. They are inputs available to ci_hsc. gen2convert should convert them so they're available to the gen3 version when processing on ci_hsc

        Attachments

          Issue Links

            Activity

            yusra Yusra AlSayyad created issue -
            yusra Yusra AlSayyad made changes -
            Field Original Value New Value
            Epic Link DM-16675 [ 235235 ]
            Hide
            nlust Nate Lust added a comment -

            You may be interested in the pattern Jim Bosch used in DM-17048 for doing something similar with ref_cats.

            Show
            nlust Nate Lust added a comment - You may be interested in the pattern Jim Bosch used in DM-17048 for doing something similar with ref_cats.
            jbosch Jim Bosch made changes -
            Link This issue is duplicated by DM-17302 [ DM-17302 ]
            Hide
            jbosch Jim Bosch added a comment -

            The first few commits, at least.  There was a lot of extra stuff for ref_cat that shouldn't be necessary for brightObjectMask.  Maybe we should pair-program this?

            Show
            jbosch Jim Bosch added a comment - The first few commits, at least.  There was a lot of extra stuff for ref_cat that shouldn't be necessary for brightObjectMask.  Maybe we should pair-program this?
            Hide
            yusra Yusra AlSayyad added a comment -

            Feel free to assign to yourself.  I see you filed a ticket right after I did!

            Show
            yusra Yusra AlSayyad added a comment - Feel free to assign to yourself.  I see you filed a ticket right after I did!
            jbosch Jim Bosch made changes -
            Assignee Yusra AlSayyad [ yusra ] Jim Bosch [ jbosch ]
            jbosch Jim Bosch made changes -
            Status To Do [ 10001 ] In Progress [ 3 ]
            Hide
            jbosch Jim Bosch added a comment -

            It turns out that the problem is a bit more interesting than I thought, and I'm inclined to fix it by changing how the regular Gen2 ci_hsc works a bit.

            The issue is that the brightObjectMasks are in the repo at $CI_HSC_DIR/DATA, and they're defined with a (tract, patch) data ID.  But there's actually no skymap defined in that repo - the skymap is added to the $CI_HSC_DIR/DATA/rerun/ci_hsc repo.  So the gen2convert is quite reasonably warning that it can't identify the skymap that defines the tract and patch data ID for brightObjectMask, and then skipping them.

            This could be fixed by either defining the skymap directly in the root repo or putting the brightObjectMasks into the rerun.  I'm leaning towards the latter; please shout if you have an opinion.

            We should also keep this in mind when it comes time to run gen2convert on the real repos on lsst-dev or tiger; I bet they suffer from the same problem.

            Show
            jbosch Jim Bosch added a comment - It turns out that the problem is a bit more interesting than I thought, and I'm inclined to fix it by changing how the regular Gen2 ci_hsc works a bit. The issue is that the brightObjectMasks are in the repo at $CI_HSC_DIR/DATA , and they're defined with a (tract, patch) data ID.  But there's actually no skymap defined in that repo - the skymap is added to the $CI_HSC_DIR/DATA/rerun/ci_hsc repo.  So the gen2convert is quite reasonably warning that it can't identify the skymap that defines the tract and patch data ID for brightObjectMask, and then skipping them. This could be fixed by either defining the skymap directly in the root repo or putting the brightObjectMasks into the rerun.  I'm leaning towards the latter; please shout if you have an opinion. We should also keep this in mind when it comes time to run gen2convert on the real repos on lsst-dev or tiger; I bet they suffer from the same problem.
            Hide
            jbosch Jim Bosch added a comment - - edited

            Changed my mind: I'm now leaning towards putting the SkyMap in the root repo, as that will let us inherit both in Gen3-only processing later instead of having to manually transfer them from the Gen2 output repo.

            Show
            jbosch Jim Bosch added a comment - - edited Changed my mind: I'm now leaning towards putting the SkyMap in the root repo, as that will let us inherit both in Gen3-only processing later instead of having to manually transfer them from the Gen2 output repo.
            Hide
            jbosch Jim Bosch added a comment -

            Changed my mind again: makeSkyMap.py won't run unless you give it an explicit output repo, so the brightObjectMasks are going in the ci_hsc rerun.

            Anyhow, in addition to those changes I've added a new Formatter class to pipe_tasks (next to the definition of the ObjectMaskCatalog class) to support reading these without lying about them being FITS the way we did in Gen2.

            Tim Jenness, could you look at the daf_butler changes?  It's some straightforward config additions, along with the removal of a bad validity check as we discussed briefly on Slack.

            Yusra AlSayyad, could you look at the pipe_tasks and ci_hsc changes?

            Show
            jbosch Jim Bosch added a comment - Changed my mind again: makeSkyMap.py won't run unless you give it an explicit output repo, so the brightObjectMasks are going in the ci_hsc rerun. Anyhow, in addition to those changes I've added a new Formatter class to pipe_tasks (next to the definition of the ObjectMaskCatalog class) to support reading these without lying about them being FITS the way we did in Gen2. Tim Jenness , could you look at the  daf_butler changes?  It's some straightforward config additions, along with the removal of a bad validity check as we discussed briefly on Slack. Yusra AlSayyad , could you look at the  pipe_tasks and  ci_hsc changes?
            jbosch Jim Bosch made changes -
            Reviewers Tim Jenness, Yusra AlSayyad [ tjenness, yusra ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            yusra Yusra AlSayyad made changes -
            Hide
            yusra Yusra AlSayyad added a comment - - edited

            Looks good. Confirmed it works (left= mask before, right= mask after)

             

            I put two commits on pipe_tasks u/yusra/DM-17300 for you to cherry-pick: https://github.com/lsst/pipe_tasks/tree/u/yusra/DM-17300  The metadata-check commit is optional.  More info on that:

            brightObjectMasks.table.getMetadata() (which in this case is ‘patch’: ‘5,4’, ‘tract’: 0}}doesn’t match the dataID which is {{'abstract_filter': 'r', 'skymap': 'ci_hsc', 'tract': 0, 'patch': 69 The code that checks the header: https://github.com/lsst/pipe_tasks/commit/a6db3fba5ff6b925e49eb8b8039fb54bb818a66c#diff-f9cf63fcf2aed29eec448bae6d243b5fR595

            The headers look like:

            [yusra@lsst-dev01 0]$ more BrightObjectMask-0-5,4-HSC-R.reg 
            # BRIGHT STAR CATALOG: Jean Coupon
            # GENERATED ON: 2017-06-04
            # TRACT: 0
            # PATCH: 5,4
            # FILTER HSC-I
            wcs; fk5
            circle(320.735301,-0.407181,0.003015d) # ID: 2686366082164587136, mag: 16.787139
            box(320.735301,-0.407181,0.000905d,0.009046d,0) # ID: 2686366082164587136, mag: 16.78713 

            and it prints this warning:

            compareWarpAssembleCoadd WARN: Expected to see abstract_filter in metadata
            compareWarpAssembleCoadd WARN: Expected to see skymap in metadata
            compareWarpAssembleCoadd WARN: Expected to see patch == 5,4 in metadata, saw 69
            

            We could either:
            (a) adapt the lines that check it so that it sees this as match
            (b) remove the lines that check it (use the second commit)
            (c) just leave it printing the warnings

            Show
            yusra Yusra AlSayyad added a comment - - edited Looks good. Confirmed it works (left= mask before, right= mask after)   I put two commits on pipe_tasks u/yusra/ DM-17300 for you to cherry-pick: https://github.com/lsst/pipe_tasks/tree/u/yusra/DM-17300   The metadata-check commit is optional.  More info on that: brightObjectMasks.table.getMetadata() (which in this case is ‘patch’: ‘5,4’, ‘tract’: 0}}doesn’t match the dataID which is {{'abstract_filter': 'r', 'skymap': 'ci_hsc', 'tract': 0, 'patch': 69 The code that checks the header: https://github.com/lsst/pipe_tasks/commit/a6db3fba5ff6b925e49eb8b8039fb54bb818a66c#diff-f9cf63fcf2aed29eec448bae6d243b5fR595 The headers look like: [yusra @lsst -dev01 0 ]$ more BrightObjectMask- 0 - 5 , 4 -HSC-R.reg # BRIGHT STAR CATALOG: Jean Coupon # GENERATED ON: 2017 - 06 - 04 # TRACT: 0 # PATCH: 5 , 4 # FILTER HSC-I wcs; fk5 circle( 320.735301 ,- 0.407181 , 0 .003015d) # ID: 2686366082164587136 , mag: 16.787139 box( 320.735301 ,- 0.407181 , 0 .000905d, 0 .009046d, 0 ) # ID: 2686366082164587136 , mag: 16.78713 and it prints this warning: compareWarpAssembleCoadd WARN: Expected to see abstract_filter in metadata compareWarpAssembleCoadd WARN: Expected to see skymap in metadata compareWarpAssembleCoadd WARN: Expected to see patch == 5,4 in metadata, saw 69 We could either: (a) adapt the lines that check it so that it sees this as match (b) remove the lines that check it (use the second commit) (c) just leave it printing the warnings
            yusra Yusra AlSayyad made changes -
            Status In Review [ 10004 ] Reviewed [ 10101 ]
            Hide
            jbosch Jim Bosch added a comment -

            Adapting the data IDs is tricky, because looking up the (camera-specific) translator for a data ID is hard to do in this context.

            I think long term I'd like to move this check to whatever is responsible for ingesting these into the data repo (right now that's just gen2convert, in which it is possible to get at those translators).  Note that in Gen3 that can't just be moving the files into position, so there will have to be some kind of script that can do this.  But I don't think we need to do that now, and I don't have a strong preference between (b) and (c) in the meantime.

            Show
            jbosch Jim Bosch added a comment - Adapting the data IDs is tricky, because looking up the (camera-specific) translator for a data ID is hard to do in this context. I think long term I'd like to move this check to whatever is responsible for ingesting these into the data repo (right now that's just gen2convert, in which it is possible to get at those translators).  Note that in Gen3 that can't just be moving the files into position, so there will have to be some kind of script that can do this.  But I don't think we need to do that now, and I don't have a strong preference between (b) and (c) in the meantime.
            Hide
            jbosch Jim Bosch added a comment -

            Merged, with (as discussed OOB) the warnings removed.

            Show
            jbosch Jim Bosch added a comment - Merged, with (as discussed OOB) the warnings removed.
            jbosch Jim Bosch made changes -
            Resolution Done [ 10000 ]
            Status Reviewed [ 10101 ] Done [ 10002 ]

              People

              • Assignee:
                jbosch Jim Bosch
                Reporter:
                yusra Yusra AlSayyad
                Reviewers:
                Tim Jenness, Yusra AlSayyad
                Watchers:
                Jim Bosch, Nate Lust, Tim Jenness, Yusra AlSayyad
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel