Fix display far in /year; decide on year definition
This MR fixes a unit conversion bug that incorrectly displayed an event's FAR in per-year units. The database value of far is always in Hz (fingers crossed), but it converted on-the-fly to per-year so it's more easily read by humans on the webpage.
This brings up a larger question, which I will leave to the masses to answer: how many days are in a year?
In today's gstlal review call @wanting.niu @jolien-creighton cited some hard-coded values in gstlal (?) and in GraceDB that fixed the numbers of days per year as the Julian value (365.25 days).
@brandon.piotrzkowski correctly pointed out that GWCelery is using the civil definition (365.0 days) for defining public alert thresholds.
Ultimately this discrepancy is only going to result in a small decimal place difference on a webpage, but we should at least be consistent. Interested parties, could you please vote below?
Question: Should GraceDB calculate the display year^-1 FAR value using the Julian or civil calendar year? Tagging some pipeline and EMFollow parties. Please place your response in a comment or edit the table with a 
| Julian, 365.25 | Civil, 365 | |
|---|---|---|
| @jolien-creighton | |
|
| @chad-hanna | ||
| @tito-canton | |
|
| @benoit.mours | |
|
| @brandon.piotrzkowski | ||
| @cody.messick |
Merge request reports
Activity
I suppose the relevant question is how many seconds are in a year, not days? Anyway, Astropy seems to default to Julian:
In [9]: from astropy.units import second, year In [10]: float(1 * year / second) Out[10]: 31557600.0 In [11]: 86400 * 365.25 Out[11]: 31557600.0PyCBC typically uses
lal.YRJUL_SIfor the conversion, so also Julian (or "Jolian" in this case? :)-
Quoting Wikipedia,
[…] it is recognized by the International Astronomical Union (IAU) as a non-SI unit for use in astronomy. […] the Julian year is defined in terms of the SI unit one second, so is as accurate as that unit and is constant. It approximates both the sidereal year and the tropical year to about ±0.008 days. The Julian year is the basis of the definition of the light-year as a unit of measurement of distance.
I think we should use the Julian year.
I've spot-checked a few of the published results. I am pretty sure that all the FARs in the GWTC papers are in units of inverse julian years, e.g., https://git.ligo.org/publications/O3/o3b-cbc-catalog/-/blob/master/data/data_process?ref_type=heads#L30 And like @tito-canton says, it seems that PyCBC uses julian years (again a spot check): https://pycbc.org/pycbc/latest/html/_modules/pycbc/events/coinc.html#LiveCoincTimeslideBackgroundEstimator.ifar I was trying to find the scripts that were used to produce FARs for GWTC-2 and GWTC-3 for GstLAL (was it Becca that produced these?) but I don't have them immediately available. I think I did this for GWTC-1 and if my memory serves I used julian years for that in alignment with the GWTC-1 paper. Again, I haven't been able to quickly locate the conversion scripts so I'm not 100% sure, but it seems that things are generally pretty consistently julian years.
So I vote for julian years.
Speaking for the LLAI review committee, although we feel strongly that there be a consistent definition of year across the entire LLAI (and the pipelines), we do not have strong feelings on which definition is used and leave that to the LL group to decide.
We also want to remind/emphasize that any/all LLAI code changes needed to fix this must be made far enough in advance to be tested properly before the start of O4b, in the manner & schedule we've all agreed to.
@pfc the "fix" in this context was because of a bug introduced in !200 (merged), which hasn't gone under review for deployment yet. It was just on the test server, for testing, where the bug was discovered, fixed, and deployed (again) to the test server.
GraceDB previously calculated /year FARs for display on the webpage using the Julian (365.25 days/year) definition, which is the default value in this merge request. So if this gets merged in the absence of any other changes, the end user result will be exactly the same as what's currently deployed and has been in place for years.
What I'm hearing is: given the reviewed code precedent that's already in place, the votes of upstream pipeline experts, and the O4b deadline approaching, I should keep this fix in place and the burden for fixing the inconsistency should go downstream? If there even is an inconsistency, correct me if i'm wrong @brandon.piotrzkowski @cody.messick @patrick.godwin.
in the manner & schedule we've all agreed to.
This is a topic for a different forum. But given the LLAI review approval timeline for https://git.ligo.org/computing/sccb/-/issues/1420, https://git.ligo.org/computing/sccb/-/issues/1451, https://git.ligo.org/computing/sccb/-/issues/1419, and https://git.ligo.org/computing/sccb/-/issues/1450, what is the manner and schedule we agreed to?
From my understanding we have already put together the release of gwcelery that will go in at the start of O4, which does not (or will not) contain the downstream changes (emfollow/gwcelery!1391) that rely on !200 (merged). So in my mind the changes in this MR isn't as urgent, but the discrepancy between the pipelines' and gwcelery definitions of a year might be.
Edited by Brandon Piotrzkowski
mentioned in merge request !203 (merged)
