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

RingsSkyMap incorrect at south pole and RA=360

    XMLWordPrintable

    Details

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

      Description

      Sogo Mineo writes:

      skyMap behaves wrongly around ra = 360^\circ. For example, the following assertion does not hold:

      skyMap = pickle.load(open("skyMap.pickle", "rb"))
      assert(9712 == skyMap.findTract(skyMap[9712].getCtrCoord()).getId())
      

      This bug is causing detect_isPrimary = false for all objects around ra = 360^\circ .
      I have no idea what's the very cause of this bug, but at least the following lines are wrong:
      https://github.com/lsst/skymap/blob/master/python/lsst/skymap/ringsSkyMap.py#L135
      When ra - self.config.raStart is near 2pi, tractNum will be 1 plus its maximum permitted value, which results in the returned tract located in the northern ring.
      EDIT: This is incorrect. The truth is tractNum starts with 1. And it's not " ra - self.config.raStart is near 2pi" but " ra - self.config.raStart is near 0" that is problematic. --End EDIT
      It should also be warned that math.fmod() in python behaves just as fmod() in C. It returns negative value when ra - self.config.raStart < 0 . In that case int(... + 0.5) should be int(... - 0.5) (But I suggest using a homemade fmod that would always return positive value) (edited)
      The fmod() misuse is irrelevant to the current problem because self.config.raStart = 0 for now.
      RingsSkyMap.getRingIndices() also behaves wrongly around the south pole.
      Let the return value (ringNum, tractNum).
      tractNum usually starts with 1.
      But from getRingIndices(index=1) is returned tractNum = 0 .
      As a result, the 0-th ring has 11 tracts while self._ringNum[0]=10 ,
      and skyMap[1] and skyMap[11] are exactly the same.

      We need to fix these problems without renumbering all the tracts because there is existing data that we don't want to invalidate.

        Attachments

        1. new.png
          new.png
          94 kB
        2. old.png
          old.png
          91 kB

          Activity

          Hide
          price Paul Price added a comment -

          I believe I've made all the changes you requested on GitHub, and am running Jenkins a final time.
          If you don't mind, I'll pass on the deprecation warning as I think it will do more harm than good: it would be generated whenever reading an old pickled skymap, and the message would scare the user unnecessarily. Instead, I intend to post an announcement to CLO once this is ready to merge.

          Show
          price Paul Price added a comment - I believe I've made all the changes you requested on GitHub, and am running Jenkins a final time. If you don't mind, I'll pass on the deprecation warning as I think it will do more harm than good: it would be generated whenever reading an old pickled skymap, and the message would scare the user unnecessarily. Instead, I intend to post an announcement to CLO once this is ready to merge.
          Hide
          rowen Russell Owen added a comment -

          That sounds great.

          Show
          rowen Russell Owen added a comment - That sounds great.
          Hide
          price Paul Price added a comment -

          Fixed an RA wraparound bug identified by Sogo Mineo in an HSC release. Jenkins has passed. I intend to merge later today.

          Show
          price Paul Price added a comment - Fixed an RA wraparound bug identified by Sogo Mineo in an HSC release. Jenkins has passed. I intend to merge later today.
          Hide
          price Paul Price added a comment -

          Merged to master. Post to CLO to follow soon.

          Show
          price Paul Price added a comment - Merged to master. Post to CLO to follow soon.
          Hide
          price Paul Price added a comment -
          Show
          price Paul Price added a comment - Posted to CLO: https://community.lsst.org/t/bug-in-ringsskymap/2996

            People

            Assignee:
            price Paul Price
            Reporter:
            price Paul Price
            Reviewers:
            Russell Owen
            Watchers:
            John Swinbank, Paul Price, Russell Owen, Sogo Mineo
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.