BUG: fix triangular interferometer coordinates

[filippo-santoliquido]

Hi,

I believe I have found two bugs in the implementation of the triangular-shaped interferometer.
The first issue concerns this test, where the angles passed to this function should be given in radians, as also stated in [1]. However, in the current implementation, the last two latitude values are passed in degrees.
When the angles are passed correctly, the test fails. For example, if the position of the first vertex is set to (40.516°, 9.416°) with x_arm_azimuth = 70.5674°, the distances between the vertices are:

ET-1 ↔️ ET-2: 9.06 km
ET-1 ↔️ ET-3: 9.76 km
ET-2 ↔️ ET-3: 7.70 km

I believe this test failure is due to an incorrect implementation of the functions updating latitude and longitude, which differ from those provided in [1]:

const φ2 = Math.asin( Math.sin(φ1)*Math.cos(d/R) +
Math.cos(φ1)*Math.sin(d/R)*Math.cos(brng) );
const λ2 = λ1 + Math.atan2(Math.sin(brng)*Math.sin(d/R)*Math.cos(φ1),
Math.cos(d/R)-Math.sin(φ1)*Math.sin(φ2));

The only difference to keep in mind is the definition of the bearing angle: in bilby it is defined counterclockwise from east, whereas in [1] it is clockwise from north. This leads to the following conversion:

brng = 90 - x_arm_azimuth

Using the above expressions, the coordinates are recovered correctly.
I also include a map illustrating this result: the blue dots represent the incorrect coordinates, while the red dots show the corrected ones.

I believe this also resolves this issue, since, when using the Virgo site coordinates as the starting point, it becomes possible to recover the positions of the other two vertices correctly, as defined in LAL.

I don’t think this bug has any practical effect on previously obtained parameter estimation results.

I’m opening this pull request to contribute to fixing this bug.

I am tagging a few people who might be interested in the matter @jacopo.tissino @sylvia.biscoveanu

Thanks a lot in advance.
Kind regards,
Filippo

[1] https://www.movable-type.co.uk/scripts/latlong.html

[ColmTalbot]

Thanks @filippo-santoliquido how does this compare with #894?

[filippo-santoliquido]

Hi Colm,

Thanks for pointing me to #894

The coordinates produced by the two methods are essentially identical

#1016 : (0.7615118398425819, 0.18333805212847526), (0.7629902506868369, 0.18405974788582488), (0.7627015830984775, 0.18192790169194423)

#894 : (0.7615118398425819, 0.18333805212847526) (0.7629930799428372, 0.1840585887020974), (0.762704632610569, 0.18192996730123293)

though the method implemented in #894 yields slightly more precise results, as it also updates the elevation. This level of precision is, of course, important for the null stream.

At this point, I think the choice of which PR to accept is up to you. My PR applies minimal changes to the current implementation, while #894 version is more precise and may be useful for specific studies.

In any case, the testing bug should be fixed regardless.

Cheers,
Filippo

[ColmTalbot]

We discussed this on a development call and the initial consensus is to get this simpler fix in soon and revist the more complex implementation at a later time.