search

gwastro / pycbc

Core package to analyze gravitational-wave data, find signals, and study their parameters. This package was used in the first direct detection of gravitational waves (GW150914), and is used in the ongoing analysis of LIGO/Virgo data.
http://pycbc.org
GNU General Public License v3.0
312 stars 347 forks source link

Optimizing calls to time_delay_from_earth_center #4434

Closed pannarale closed 4 weeks ago

pannarale commented 1 year ago

The call to time_delay_from_earth_center in pycbc_multi_inspiral is inserted in a triple loop on ifos, sky positions and time slides, but the time delay depends only on the IFO and the sky position.

Specifically, https://github.com/gwastro/pycbc/blob/master/bin/pycbc_multi_inspiral#L223-L224 are executed N_IFO N_skypoints N_slides times rather than N_IFO N_skypoints times. This needs to be handled differently so that we end up with N_IFO N_skypoints calls only and the timeslides are folded in separately.

This one is for Sebastian Gomez Lopez (@sebastiangomezlopez)

pannarale commented 1 year ago

@sebastiangomezlopez, can you please compare https://github.com/pannarale/pycbc/tree/time_delay_optimization to the current code in terms of

  1. the plot with run time vs number of slides, and
  2. the profiling of the specific job you have for the current code?
pannarale commented 1 year ago

Before the change vs after the change for a single point search! Thanks, @sebastiangomezlopez

Image

titodalcanton commented 1 year ago

It may be beneficial (in terms of memory and/or computing time, though maybe not code clarity) to switch from a nested dict data structure to a 3-dimensional Numpy array for storing quantites that depend on (detector, position, slide).

If you do, I suggest writing a comment that explains the intent of each dimension of the 3D array clearly.

pannarale commented 1 year ago

We will try that and add a third curve to the performance plot.

spxiwh commented 1 year ago

It should be possible to call time_delay_from_earth_center once and have it compute N_IFO * N_skypoints quantities. I think it could take vectorized input (might need some poking).

If that doesn't work, this could be cythonized, but I think it should be able to take vectorized inputs.

... But it would be good to post profile graphs here (I highly recommend gprof2dot (https://github.com/jrfonseca/gprof2dot) for making these).

titodalcanton commented 4 weeks ago

This has been fixed in #4710.