comments on the multi_inspiral executable #4437
Conversation
bin/pycbc_multi_inspiral
Outdated
| * norm) # snr time series for each ifo | ||
| norm_dict[ifo] = norm | ||
| corr_dict[ifo] = corr.copy() # The correlation vector for each | ||
| # ifo. it is the FFT of the snr | ||
| # (so inverse FFT it to get the snr). | ||
| idx[ifo] = ind.copy() # Trigger indices list for each ifo | ||
| snrv_dict[ifo] = snrv.copy() # Trigger snr list for each ifo | ||
| snr[ifo] = snrv * norm # Normalized trigger snr list for each ifo |
There was a problem hiding this comment.
Personally I find inline comments like these a bit hard to read, I suggest putting them in separate lines before the code they refer to.
bin/pycbc_multi_inspiral
Outdated
| logging.info("Making frequency-domain data segments") | ||
| segments = { | ||
| ifo: strain_segments_dict[ifo].fourier_segments() | ||
| for ifo in args.instruments | ||
| } | ||
| del strain_segments_dict | ||
| del strain_segments_dict # The timedomain segments dictionary is deleted |
There was a problem hiding this comment.
This comment does not add much: the syntax of the code is obvious. A more useful comment would state why we delete that object at this point. Is it to save memory, or to avoid confusion…?
Also see my other comment below, I prefer comments on a separate line before the code.
bin/pycbc_multi_inspiral
Outdated
| # relevant inputs for segment computation: | ||
| # --trig-end-time | ||
| # --trig-start-time | ||
| # --segment-length | ||
| # --segment-start-pad | ||
| # --segment-end-pad | ||
| # NOTE: --segment-length 128 breaks the | ||
| # master/examples/multi_inspiral/run.sh example | ||
| # | ||
| # assert cases when seg_pad exceeds the seg_len: | ||
| # certain combinations of --segment-start-pad, | ||
| # --segment-end-pad & --segment-length cause the | ||
| # segment_width computed in master/pycbc/strain/strain.py | ||
| # to have values <=0. |
There was a problem hiding this comment.
What is the point of the indentation here? It is using a lot of space.
bin/pycbc_multi_inspiral
Outdated
| # The following block of lines sets up the command-line interface (CLI) for the | ||
| # pycbc_multi_inspiral executable. To see an example of how to feed this executable | ||
| # to analyze the GW170817 event, take a look at: | ||
| # https://github.com/gwastro/pycbc/blob/master/examples/multi_inspiral/run.sh |
There was a problem hiding this comment.
Please keep two blank lines after the imports (standard PEP8 style).
I would put the URL of the example script in the docstring above.
sebastiangomezlopez
force-pushed
the
master
branch
from
d6381d9
to
61c9f0c
Compare
Hi,
As discussed with @pannarale and @titodalcanton. I worked on commenting the pycbc_multi_inspiral executable in order to make clearer what the executable does, highlight where certain computations are happening, and mention a few possible fixes/optimizations that current developers could work on in the future.