Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: skymap
-
Labels:None
-
Team:External
Description
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.
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.