raven issueshttps://git.ligo.org/lscsoft/raven/-/issues2024-03-26T17:47:10Zhttps://git.ligo.org/lscsoft/raven/-/issues/44Separate out explicit coinc FAR calculation so can be used offline more easily2024-03-26T17:47:10ZBrandon PiotrzkowskiSeparate out explicit coinc FAR calculation so can be used offline more easilyCurrently we have to mock up GraceDB just to do basic joint FAR calculations. Instead we should allocate this to a separate internal function that will be called by the current one, but also callable separately.Currently we have to mock up GraceDB just to do basic joint FAR calculations. Instead we should allocate this to a separate internal function that will be called by the current one, but also callable separately.ligo-raven 4.0Brandon PiotrzkowskiBrandon Piotrzkowskihttps://git.ligo.org/lscsoft/raven/-/issues/41Use method to correct bias in joint FAR w/ sky map info2024-03-28T19:45:37ZBrandon PiotrzkowskiUse method to correct bias in joint FAR w/ sky map infoTaken as a limitation since O3 the current method of, it is known that the current method of calculating the joint FAR using the sky map overlap has a bias and lower than expected number of low-significance triggers. See for more details...Taken as a limitation since O3 the current method of, it is known that the current method of calculating the joint FAR using the sky map overlap has a bias and lower than expected number of low-significance triggers. See for more details: https://git.ligo.org/brandon.piotrzkowski/brandon-piotrzkowski-phd-thesis/-/jobs/artifacts/main/file/main.pdf?job=pdf
Suggestions, such as in https://dcc.ligo.org/LIGO-T2300398, could be implemented to correct this bias.ligo-raven 4.1https://git.ligo.org/lscsoft/raven/-/issues/38Review comments on GraceDB interface code, and general inspection of all the ...2024-03-01T21:48:52ZTito Dal CantonReview comments on GraceDB interface code, and general inspection of all the code baseSorry 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 n...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.
- [x] 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.
- [x] Having default kw args set to `[]` is dangerous, see for example the discussion [here](https://nikos7am.com/posts/mutable-default-arguments/). 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?
- [x] In `search.py`, is there any reason why [this](https://git.ligo.org/lscsoft/raven/-/blob/master/ligo/raven/search.py#L75-76) 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.
- [x] [This message](https://git.ligo.org/lscsoft/raven/-/blob/master/ligo/raven/search.py#L486-487) 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.
- [x] [Here](https://git.ligo.org/lscsoft/raven/-/blob/master/ligo/raven/search.py#L652) 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.
- [x] 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.
- [x] 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.ligo-raven 4.0Brandon PiotrzkowskiNaresh AdhikariBrandon Piotrzkowskihttps://git.ligo.org/lscsoft/raven/-/issues/37Further review comments on skymap overlap2024-03-01T21:49:41ZTito Dal CantonFurther review comments on skymap overlapSorry for being so late on this. Here are a few more comments from looking at the skymap overlap *tests*. None of them warrant holding up the May 10 signoff.
## Overall
- [ ] `True if 'multiorder' in xyz_filename else False` can be sho...Sorry for being so late on this. Here are a few more comments from looking at the skymap overlap *tests*. None of them warrant holding up the May 10 signoff.
## Overall
- [ ] `True if 'multiorder' in xyz_filename else False` can be shortened to `'multiorder' in xyz_filename`.
## Related to `test_overlap_integrals_moc()`
- [x] `io.read_sky_map()` is only called with `nest=True` if the skymap uses MOC. According to the [docstring](https://git.ligo.org/lscsoft/ligo.skymap/-/blob/main/ligo/skymap/io/fits.py#L397-402) of that function, omitting `nest=True` will cause the returned skymap to use ring ordering. So it appears the skymap will use nest or ring ordering depending on whether it was MOC or not. However, later, `skymap_overlap_integral()` is always passed `se_nested=True, ext_nested=True`. I think the logic in `skymap_overlap_integral()` leads to the correct result anyway, but this may look like a mistake to a non-expert eye. Perhaps a comment would help.
- [x] More interestingly, this function does not test calling `skymap_overlap_integral()` in "infinite precision" more, i.e. using `ra` and `dec`. That case is tested further down in `test_overlap_integrals_NGC4993()`. However, for good measure it should be trivial to add a few test cases here as well, because a "Dirac delta skymap" and a flat skymap should still give you 1.
- [ ] I would also add a few test cases that evaluate to zero, for example a flat hemisphere compared to a single point in the other hemisphere, and two disjoint small flat patches. The latter should be easy to create by setting a single nonzero pixel in a zero HEALPix array.
## Related to `test_overlap_integrals_GW170817()`
- [ ] This function is essentially identical to `test_overlap_integrals_moc()`. If you add more parameters to the `pytest.mark.parametrize` decorator, to hold the path and expected result, you can just add these cases as extra cases in the previous function.
- [ ] [Ashton et al 2018](https://iopscience.iop.org/article/10.3847/1538-4357/aabfd2) reports 32.4 for the GW170817 overlap, while the test expects 32.286. The difference is probably to be expected (who knows which skymaps we used back then) however a comment pointing to the paper for reference would be nice.ligo-raven 4.0Brandon PiotrzkowskiNaresh AdhikariBrandon Piotrzkowskihttps://git.ligo.org/lscsoft/raven/-/issues/30Add ability to use more informative prior2024-03-22T20:38:47ZBrandon PiotrzkowskiAdd ability to use more informative priorCurrently we use a simple uniform prior (4 pi analytically; 1/N_pix where N_pix is the number of pixels numerically).
However we could include some improvements:
- [ ] Include antenna factors of instruments involved in detection
- [ ] I...Currently we use a simple uniform prior (4 pi analytically; 1/N_pix where N_pix is the number of pixels numerically).
However we could include some improvements:
- [ ] Include antenna factors of instruments involved in detection
- [ ] Include the occultation of earth (and sun and moon?) for the specific GRB experiment at the time.ligo-raven 4.1Naresh AdhikariNaresh Adhikarihttps://git.ligo.org/lscsoft/raven/-/issues/22Incorporate GW-FRB search with CHIME2024-03-22T20:38:47ZBrandon PiotrzkowskiIncorporate GW-FRB search with CHIMESince CHIME VOEvents are now available, we will want to be able to incorporate a GW-FRB search into RAVEN. This requires the following:
- [ ] Determine whether the [-2,+12]s search window used elsewhere in the FRB searches (PyGRB and X-p...Since CHIME VOEvents are now available, we will want to be able to incorporate a GW-FRB search into RAVEN. This requires the following:
- [ ] Determine whether the [-2,+12]s search window used elsewhere in the FRB searches (PyGRB and X-pipeline) should be used here as well
- [ ] Calculate the approximate number of CHIME events per year
- [ ] Determine how sky map information from CHIME can be incorporated
- [x] Ask Alex Pace to add `CHIME` and `FRB` fields to GraceDB (pipeline and search values; ingestion of FRB VOEvent)
A related issue is #20.ligo-raven 4.1Naresh AdhikariNaresh Adhikarihttps://git.ligo.org/lscsoft/raven/-/issues/20Incorporate distance information in overlap integral2024-03-22T20:38:47ZBrandon PiotrzkowskiIncorporate distance information in overlap integralDue to some requests from FRB searches, there has been a need to incorporate distance information into the overlap integral.
Some preliminary work has been done on this, using a galaxy catalogue: https://git.ligo.org/brandon.piotrzkowsk...Due to some requests from FRB searches, there has been a need to incorporate distance information into the overlap integral.
Some preliminary work has been done on this, using a galaxy catalogue: https://git.ligo.org/brandon.piotrzkowski/gw-grb-rankingstat/-/blob/master/GW170817/calculate-overlap-galaxy-catalogue-condor.py#L411-442
However, the intent is to use the distance info from CHIME to help additionally distinguish real from false coincidences. See here for a discussion of the CHIME VOEvent schema: https://chime-frb-open-data.github.io/static/CHIMEFRB_VOEvent_Service_for_External_Subscribers.pdfligo-raven 4.1Naresh AdhikariNaresh Adhikari