Review comments on GraceDB interface code, and general inspection of all the code base

Sorry again for being so late. Here are some comments related to the usage of gracedb-sdk in parallel with ligo-gracedb, and more generally comments on the main code. None of this warrants holding up the May 10 signoff, but it would be nice to address in future releases.

Having to support both APIs clearly adds significant complexity to an otherwise very simple code base. This is a sad situation to witness, but whatever. I think the way this has been handled in RAVEN is pretty clean.

• Where are you actually importing gracedb-sdk? All GraceDB imports I see are importing ligo-gracedb. I guess the point is that you would be able to change the imports to gracedb-sdk if you wanted to, and everything would continue working? Also, the code can take a GraceDB client object as parameter, and the user could pass anything there. So ok, this looks good.

• Having default kw args set to [] is dangerous, see for example the discussion here. Those defaults should be replaced with None and then, if necessary, None can be replaced with [] in the function's body.

• There is significant code duplication in gracedb_events.py: the ExtTrig and SE classes basically do the same things. I think this warrants a parent class which is subclassed for the specific behavior of external events and superevents.

• In gracedb_events.py, I am not sure I understand why the is_moc parameter is necessary. The code actually reads the FITS file of the skymap, so why can it not simply find out on its own if the skymap is using MOC or not?

• In search.py, is there any reason why this is not simply an exception? Is it because of how it has to interact with the GraceDB log?

• When formatting the GraceDB comments that explain the results, you could use f-strings to make it a bit easier to see what is going on.

• This message would be wrong if the FAR happened to be zero. I suggest checking all possible invalid FAR values explicitly (None, zero and negative) and providing appropriate error messages.

• Here raising a ZeroDivisionError is not necessarily appropriate, as the actual failure could come from other kinds of problems. I would raise a generic RuntimeError here.

• mock_gracedb_sdk.py and mock_gracedb_rest.py seem to duplicate the mock event definitions. Maybe you could store the mock event definitions in a single JSON file (also easier to maintain, if we want to add more mock info), and have those two Python modules read the same file.

• In test_raven_search.py, it seems the from ligo.gracedb import rest import can be dropped. I recommend using a linter to check for other similar cases.

• In test_raven_search.py, note that you can "stack" the @pytest.mark.parametrize decorator multiple times:

@pytest.mark.parametrize("x", [0, 1])
@pytest.mark.parametrize("y", [2, 3])
def test_foo(x, y):
    pass

to test the Cartesian product of all cases. This may reduce the length of the test case lists.

• Among the tested mock event GPS times, I would add some actual GPS times corresponding to the present, or maybe even times in the next decade, to test for possible overflow/rounding errors with large numbers.