Review comments on skymap overlap code
I started looking at https://git.ligo.org/lscsoft/raven/-/blob/d8ad373a530e9966e2ff93030c1cfa01db6035b8/ligo/raven/search.py#L257. I am leaving some comments here. I will append as I keep going.
-
The function contains a nest of logical branches handling all the various possible input formats. Following the spirit of "leaving the code as soon as possible", I would suggest replacing the assignments of the skymap_overlap_integral
variable with return statements. I would then raise a ValueError at the end of the function, to catch a case that should nominally never be reached. -
In a few places, the function tests whether the ra
anddec
argument are being provided by converting them to booleans, e.g.…and not (ra and dec)
. This check will fail ifra = 0.
ordec = 0.
. The correct check isra is not None
/dec is not None
. Also, it would conceptually be more appropriate to passra
anddec
as a single object, maybe directly aSkyCoord
, but if changing this is a lot of work, no worries. -
L384 prints a warning indicating a potentially incorrect result. Would this warning show up on GraceDB? What would we do in that case? Would it be better to return 1 in this case (as in "no information") as opposed to a potentially invalid result?
Edited by Tito Dal Canton