Skip to content

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 and dec argument are being provided by converting them to booleans, e.g. …and not (ra and dec). This check will fail if ra = 0. or dec = 0.. The correct check is ra is not None / dec is not None. Also, it would conceptually be more appropriate to pass ra and dec as a single object, maybe directly a SkyCoord, 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