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

Cleanup ci_hsc_gen2 to use new convert script instead of custom one

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: ci_hsc
    • Labels:
      None
    • Story Points:
      2
    • Sprint:
      AP S20-3 (February), AP S20-4 (March)
    • Team:
      Alert Production
    • Urgent?:
      No

      Description

      In order to help get DM-22655 merged sooner, I'm going to move the work of converting ci_hsc to use the new script onto this ticket, instead of doing it there. I'd already started on that process, and will move that work onto this ticket.

        Attachments

          Issue Links

            Activity

            Hide
            tjenness Tim Jenness added a comment -

            John Parejko has asked me to take the ticket out of reviewed state because I don't think the ticket should be merged when it is so much slower than the previous implementation.

            Looking at the code the only difference I can see between the new and the old version is that the obs_base conversion script does not set rootSkyMapName in the config. Maybe that causes the problem?

            Show
            tjenness Tim Jenness added a comment - John Parejko has asked me to take the ticket out of reviewed state because I don't think the ticket should be merged when it is so much slower than the previous implementation. Looking at the code the only difference I can see between the new and the old version is that the obs_base conversion script does not set rootSkyMapName in the config. Maybe that causes the problem?
            Hide
            Parejkoj John Parejko added a comment -

            Thanks for that suggestion, Tim Jenness: I don't understand why `rootSkyMapName` mattered (the skymap config and name was already specified in `config.skymaps`), but it appears to have cut the convert runtime down to about a minute.

            Show
            Parejkoj John Parejko added a comment - Thanks for that suggestion, Tim Jenness : I don't understand why `rootSkyMapName` mattered (the skymap config and name was already specified in `config.skymaps`), but it appears to have cut the convert runtime down to about a minute.
            Hide
            tjenness Tim Jenness added a comment - - edited

            That's great. I haven't looked too deeply at the code but I saw a bunch of "if rootSkyMapName is None" shenanigans going on suggesting that different code paths would be involved.

            Show
            tjenness Tim Jenness added a comment - - edited That's great. I haven't looked too deeply at the code but I saw a bunch of "if rootSkyMapName is None" shenanigans going on suggesting that different code paths would be involved.
            Hide
            jbosch Jim Bosch added a comment -

            I saw a bunch of "if rootSkyMapName is None" shenanigans going on suggesting that different code paths would be involved.

            This is a workaround for the fact that a Gen2 repo can have datasets defined on tract and patch without having a skymap dataset that defines what those mean.  It's a totally ill-formed repository configuration, and apparently we get away with it because people just guess that a skymap dataset in a child repository is the right one for the parent.  That config option sets what skymap to guess when the information simply isn't in the Gen2 repo.

            Show
            jbosch Jim Bosch added a comment - I saw a bunch of "if rootSkyMapName is None" shenanigans going on suggesting that different code paths would be involved. This is a workaround for the fact that a Gen2 repo can have datasets defined on tract and patch without having a skymap dataset that defines what those mean.  It's a totally ill-formed repository configuration, and apparently we get away with it because people just guess that a skymap dataset in a child repository is the right one for the parent.  That config option sets what skymap to guess when the information simply isn't in the Gen2 repo.
            Hide
            Parejkoj John Parejko added a comment -

            Jenkins failure due to the bug introduced by DM-24285, but I think everything in this ticket is ok. I've merged it to get this done with. If there is a bug hiding because of the problem in DM-24285, let me know and I'll fix it ASAP.

            Show
            Parejkoj John Parejko added a comment - Jenkins failure due to the bug introduced by DM-24285 , but I think everything in this ticket is ok. I've merged it to get this done with. If there is a bug hiding because of the problem in DM-24285 , let me know and I'll fix it ASAP.

              People

              • Assignee:
                Parejkoj John Parejko
                Reporter:
                Parejkoj John Parejko
                Reviewers:
                Tim Jenness
                Watchers:
                Jim Bosch, John Parejko, John Swinbank, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel