[[_TOC_]]
# Code Review
[Diff against master](https://git.ligo.org/nv.krishnendu/lalsuite/compare/master...Ks_21March2019)
## Overall code status
| Code | Details | Status |
|------|--------|--------|
| `lalinference/python/cbcBayesCompPos.py` | [Details](https://git.ligo.org/nv.krishnendu/lalsuite/wikis/Code-review#lalinferencepythoncbcbayescomppospy)|:white_check_mark: |
| `lalinference/python/cbcBayesPostProc.py `|[Details](https://git.ligo.org/nv.krishnendu/lalsuite/wikis/Code-review#lalinferencepythoncbcbayespostprocpy-and-lalinferencepythonlalinferencebayespputilspy)|:white_check_mark:|
| `lalinference/python/lalinference/bayespputils.py` |[Details](https://git.ligo.org/nv.krishnendu/lalsuite/wikis/Code-review#lalinferencepythoncbcbayespostprocpy-and-lalinferencepythonlalinferencebayespputilspy)|:white_check_mark:|
| `lalinference/src/LALInference.c` |[Details](https://git.ligo.org/nv.krishnendu/lalsuite/wikis/Code-review#lalinferencesrclalinferencec-and-lalinferencesrclalinferenceh)|:white_check_mark:|
| `lalinference/src/LALInference.h` |[Details](https://git.ligo.org/nv.krishnendu/lalsuite/wikis/Code-review#lalinferencesrclalinferencec-and-lalinferencesrclalinferenceh)|:white_check_mark:|
| `lalinference/src/LALInferenceInitCBC.c` |[Details](https://git.ligo.org/nv.krishnendu/lalsuite/wikis/Code-review#lalinferencesrclalinferenceinitcbcclalinferencesrclalinferenceproposalclalinferencesrclalinferencetemplatec)|:white_check_mark:|
| `lalinference/src/LALInferenceProposal.c` |[Details](https://git.ligo.org/nv.krishnendu/lalsuite/wikis/Code-review#lalinferencesrclalinferenceinitcbcclalinferencesrclalinferenceproposalclalinferencesrclalinferencetemplatec)|:white_check_mark:|
| `lalinference/src/LALInferenceTemplate.c` |[Details](https://git.ligo.org/nv.krishnendu/lalsuite/wikis/Code-review#lalinferencesrclalinferenceinitcbcclalinferencesrclalinferenceproposalclalinferencesrclalinferencetemplatec)|:white_check_mark:|
| `lalsimulation/lib/LALSimInspiralTestGRParams.c`| [Details](https://git.ligo.org/nv.krishnendu/lalsuite/-/wikis/Code-review#additional-code-change) | :white_check_mark: |
| Sampling Sanity checks|[Details](https://git.ligo.org/nv.krishnendu/lalsuite/wikis/Code-review#sampling-checks)|:white_check_mark: |
UPDATE: The final version of the code removes the print statements inside `lalinference/src/LALInferenceTemplate.c`. The corresponding patch is **here**.
## Build details
| Branch | Git hash | Diff Patch | Build status |
|--------|----------|------------|--------------|
| [Ks_21March2019](https://git.ligo.org/nv.krishnendu/lalsuite/commits/Ks_21March2019) | [7ec73781](https://git.ligo.org/nv.krishnendu/lalsuite/commit/7ec7378129a05b343dcf5d088f97426e41321949) | [diff.patch](https://git.ligo.org/nv.krishnendu/lalsuite/blob/7ec7378129a05b343dcf5d088f97426e41321949/diff.patch) | SUCCESSFUL |
## Codes
### `lalinference/python/cbcBayesCompPos.py`
| Tasks| Results| Status |
|---|---|---|
| Comparison of GR case between master and feature branch |[Comparison page](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/siqm_master_comparison/) |:white_check_mark: |
| Cases that cover all 4 non-GR parameters | [KsKa](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/kska_run1_run2_comparison/)
[K1K2](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/k1k2_run1_run2_comparison/) |:white_check_mark:|
### `lalinference/python/cbcBayesPostProc.py` and `lalinference/python/lalinference/bayespputils.py`
| Tasks| Results | Status |
|---|---|---|
| Binning appropriate | [dquadmon1.png](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/k1k2/900000000-0/V1H1L1/1Dpdf/dquadmon1.png)
[dquadmon2.png](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/k1k2/900000000-0/V1H1L1/1Dpdf/dquadmon2.png)
[dquadmons.png](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/kska/900000000-0/V1H1L1/1Dpdf/dquadmons.png)
[dquadmona.png](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/kska/900000000-0/V1H1L1/1Dpdf/dquadmona.png) | :white_check_mark: |
| Labels correct | Same as above |:white_check_mark: |
### `lalinference/src/LALInference.c` and `lalinference/src/LALInference.h`
| Tasks| Status |
|---|---|
| Output of `LALInferencedQuadMonSdQuadMonA` function|:white_check_mark: |
We use the following python script to check the output of the `LALInferencedQuadMonSdQuadMonA` function. We generate samples from a distribution uniform in dQuadMonS and dQuadMonA, and plot the scatter of the corresponding dQuadMon1, dQuadMon2 values.
```
dQuadMonS = np.random.uniform(-200,200,10000)
dQuadMonA = np.random.uniform(-200,200,10000)
dQuadMon1, dQuadMon2 = [],[]
for idx in range(10000):
x = li.dQuadMonSdQuadMonA(dQuadMonS[idx], dQuadMonA[idx])
dQuadMon1.append(x[0]), dQuadMon2.append(x[1])
plt.figure()
plt.scatter(dQuadMonS, dQuadMonA, color='r', alpha=0.1, label='S-A')
plt.scatter(dQuadMon1, dQuadMon2, color='k', alpha=0.05, label='1-2')
plt.legend(loc='best')
plt.xlabel('$\delta \kappa _S$')
plt.ylabel('$\delta \kappa _A$')
```

### `lalinference/src/LALInferenceInitCBC.c`/`lalinference/src/LALInferenceProposal.c`/`lalinference/src/LALInferenceTemplate.c`
| | Tasks| Status |
|---|---|---|
| 1. | fix-params should fix the posterior at tmp-value |:white_check_mark: |
| 2. | `fprintf(stdout,"Both options (from 12 and SA) are given . . Exiting . .\n");` | :white_check_mark: |
| 3. | `fprintf(stdout,"Only one option (from 12 and SA) is given . . Working fine . .\n");` | :white_check_mark:|
| 4. | `fprintf(stdout,"Both dQuadMonS and dQaudMonA are sampled");` | :white_check_mark: |
| 5. | `fprintf(stdout,"dQM1: %e, dQM2: %e, dQMS: %e, dQMA: %e \n",dQuadMon1,dQuadMon2,dQuadMonS,dQuadMonA);`| :white_check_mark: |
| 6. | `fprintf(stdout,"Both dQuadMon1 and dQaudMon2 are sampled");`| :white_check_mark:|
| 7. | `fprintf(stdout,"dQM1: %e, dQM2: %e \n",dQuadMon1,dQuadMon2);` | :white_check_mark: |
**Different sampling scenarios:**
| Sampling case| Options to be passed |
|---|---|
| SA | --dQuadMonSA --dQuadMonS-min WW --dQuadMonS-max XX --dQuadMonA-min YY --dQuadMonA-max ZZ |
| S | --dQuadMonSA --fix-dQuadMonA 0.0 --dQuadMonS-min WW --dQuadMonS-max XX |
| S | --dQuadMonSA --fix-dQuadMonS 0.0 --dQuadMonA-min WW --dQuadMonA-max XX |
| 12 | --dQuadMon12 --dQuadMon1-min WW --dQuadMon1-max XX --dQuadMon2-min YY --dQuadMon2-max ZZ |
| 1 | --dQuadMon12 --fix-dQuadMon2 0.0 --dQuadMon1-min WW --dQuadMon1-max XX |
| 2 | --dQuadMon12 --fix-dQuadMon1 0.0 --dQuadMon2-min WW --dQuadMon2-max XX |
Reference Injection file: [S190814bv.xml](uploads/1e189529dbc83ded65aac7a1bfca84a2/S190814bv.xml)
#### Task 2
LALInference Command and output:
```
lalinference_nest --L1-flow 20 --approx IMRPhenomPv2threePointFivePN --psdlength 492.0 --V1-timeslide 0 --V1-cache LALSimAdVirgo --chirpmass-max 9.519249 --nlive 1024 --inj S190814bv.xml --comp-max 54.40873232560802 --adapt-temps --srate 4096.0 --event 0 --H1-timeslide 0 --V1-fhigh 135.811 --neff 1000 --seglen 16.0 --L1-channel L1:LDAS-STRAIN --L1-fhigh 135.811 --stance-max 500 --0noise --trigtime 900000000 --tol 1.0 --psdstart 899999474.0 --H1-cache LALSimAdLIGO --progress --tolerance 0.1 --H1-channel H1:LDAS-STRAIN --V1-channel V1:h_16384Hz --comp-min 1.6378358668107575 --resume --V1-flow 20 --fref 20 --outfile lalinferencenest-0-V1H1L1-900000000.0-0.hdf5 --L1-cache LALSimAdLIGO --randomseed 1732024500 --dataseed 3000 --L1-timeslide 0 --q-min 0.055728090000841224 --chirpmass-min 5.141979 --H1-flow 20 --H1-fhigh 135.11 --ifo V1 --ifo H1 --ifo L1 --dQuadMon12 --dQuadMonSA
Both options (from 12 and SA) are given . . Exiting . .
Error: cannot use more than one of --dQuadMon12 and --dQuadMonSA.
XLAL Error - LALInferenceInitCBCModel (LALInferenceInitCBC.c:1451): Invalid argument
Segmentation fault: 11
```
### Task 3-7
**--dQuadMon12**
LALInference Command and output:
```
lalinference_nest --L1-flow 20 --approx IMRPhenomPv2threePointFivePN --psdlength 492.0 --V1-timeslide 0 --V1-cache LALSimAdVirgo --chirpmass-max 9.519249 --nlive 1024 --inj S190814bv.xml --comp-max 54.40873232560802 --adapt-temps --srate 4096.0 --event 0 --H1-timeslide 0 --V1-fhigh 135.811 --neff 1000 --seglen 16.0 --L1-channel L1:LDAS-STRAIN --L1-fhigh 135.811 --stance-max 500 --0noise --trigtime 900000000 --tol 1.0 --psdstart 899999474.0 --H1-cache LALSimAdLIGO --progress --tolerance 0.1 --H1-channel H1:LDAS-STRAIN --V1-channel V1:h_16384Hz --comp-min 1.6378358668107575 --resume --V1-flow 20 --fref 20 --outfile lalinferencenest-0-V1H1L1-900000000.0-0.hdf5 --L1-cache LALSimAdLIGO --randomseed 1732024500 --dataseed 3000 --L1-timeslide 0 --q-min 0.055728090000841224 --chirpmass-min 5.141979 --H1-flow 20 --H1-fhigh 135.11 --ifo V1 --ifo H1 --ifo L1 --dQuadMon12
Read end time 900000000.000000
Using detector-based sky frame
Only one option (from 12 and SA) is given . . Working fine . .
--- ---
Templates will run with precessing spins
Templates will run using Approximant 72 (IMRPhenomPv2), phase order 7, amp order -1, spin order -1 tidal order -1 in the frequency domain.
--- ---
Template function called is "LALInferenceTemplateXLALSimInspiralChooseWaveform"
set tolerance.
Sprinkling 1024 live points, may take some time
Both dQuadMon1 and dQaudMon2 are sampleddQM1: 1.506288e+02, dQM2: -1.427024e+02
Both dQuadMon1 and dQaudMon2 are sampleddQM1: -4.831145e+01, dQM2: -5.314133e+01
Both dQuadMon1 and dQaudMon2 are sampleddQM1: -9.102365e+01, dQM2: -7.010662e+01
Both dQuadMon1 and dQaudMon2 are sampleddQM1: -9.118437e+00, dQM2: -2.130467e+01
```
**--dQuadMonSA**
LALInference Command and output:
```
Using end time from injection file: 900000000.000000
Read end time 900000000.000000
Using detector-based sky frame
Only one option (from 12 and SA) is given . . Working fine . .
--- ---
Templates will run with precessing spins
Templates will run using Approximant 72 (IMRPhenomPv2), phase order 7, amp order -1, spin order -1 tidal order -1 in the frequency domain.
--- ---
Template function called is "LALInferenceTemplateXLALSimInspiralChooseWaveform"
set tolerance.
Sprinkling 1024 live points, may take some time
Both dQuadMonS and dQaudMonA are sampleddQM1: 7.926428e+00, dQM2: 2.933312e+02, dQMS: 1.506288e+02, dQMA: -1.427024e+02
Both dQuadMonS and dQaudMonA are sampleddQM1: -1.014528e+02, dQM2: 4.829881e+00, dQMS: -4.831145e+01, dQMA: -5.314133e+01
Both dQuadMonS and dQaudMonA are sampleddQM1: -1.611303e+02, dQM2: -2.091703e+01, dQMS: -9.102365e+01, dQMA: -7.010662e+01
Both dQuadMonS and dQaudMonA are sampleddQM1: -3.042310e+01, dQM2: 1.218623e+01, dQMS: -9.118437e+00, dQMA: -2.130467e+01
```
**--dQuadMon12 --fix-dQuadMon1 0.0**
LALInference Command and output:
```
lalinference_nest --L1-flow 20 --approx IMRPhenomPv2threePointFivePN --psdlength 492.0 --V1-timeslide 0 --V1-cache LALSimAdVirgo --chirpmass-max 9.519249 --nlive 1024 --inj S190814bv.xml --comp-max 54.40873232560802 --adapt-temps --srate 4096.0 --event 0 --H1-timeslide 0 --V1-fhigh 135.811 --neff 1000 --seglen 16.0 --L1-channel L1:LDAS-STRAIN --L1-fhigh 135.811 --stance-max 500 --0noise --trigtime 900000000 --tol 1.0 --psdstart 899999474.0 --H1-cache LALSimAdLIGO --progress --tolerance 0.1 --H1-channel H1:LDAS-STRAIN --V1-channel V1:h_16384Hz --comp-min 1.6378358668107575 --resume --V1-flow 20 --fref 20 --outfile lalinferencenest-0-V1H1L1-900000000.0-0.hdf5 --L1-cache LALSimAdLIGO --randomseed 1732024500 --dataseed 3000 --L1-timeslide 0 --q-min 0.055728090000841224 --chirpmass-min 5.141979 --H1-flow 20 --H1-fhigh 135.11 --ifo V1 --ifo H1 --ifo L1 --dQuadMon12 --fix-dQuadMon1 0.0
Read end time 900000000.000000
Using detector-based sky frame
Only one option (from 12 and SA) is given . . Working fine . .
--- ---
Templates will run with precessing spins
Templates will run using Approximant 72 (IMRPhenomPv2), phase order 7, amp order -1, spin order -1 tidal order -1 in the frequency domain.
--- ---
Template function called is "LALInferenceTemplateXLALSimInspiralChooseWaveform"
set tolerance.
Sprinkling 1024 live points, may take some time
Both dQuadMon1 and dQaudMon2 are sampleddQM1: 0.000000e+00, dQM2: -1.259211e+02
Both dQuadMon1 and dQaudMon2 are sampleddQM1: 0.000000e+00, dQM2: -4.098038e+01
Both dQuadMon1 and dQaudMon2 are sampleddQM1: 0.000000e+00, dQM2: -7.584580e+01
```
* Tried for the other 3 cases as well, with identical results:
* --dQuadMonSA --fix-dQuadMonA 0.0
* --dQuadMonSA --fix-dQuadMonS 0.0
* --dQuadMon12 --fix-dQuadMon2 0.0
### Basic Sanity Checks
|Tasks|Results|Status|
|--|--|--|
|Reproducing GR results from master| [BH-master](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/bh_master/900000000-0/V1H1L1/posplots.html)
[BH-SIQM](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/bh/900000000-0/V1H1L1/posplots.html)
[Comparison page](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/siqm_master_comparison/)|:white_check_mark: |
|Zero-loglikelihood runs to check nature of all 4 priors | [K1-K2](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/zerolikelihood/k1k2/900000000-0/V1H1L1/posplots.html)
[Ks-Ka](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/zerolikelihood/kska/900000000-0/V1H1L1/posplots.html) | :white_check_mark:|
| A restricted prior reproduced GR result | [GR:feature-branch](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/bh/900000000-0/V1H1L1/posplots.html)
[GR:master](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/bh_master/900000000-0/V1H1L1/posplots.html)
[nonGR:tight-prior](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/ks_lesser_prior_21March2020/900000000-0/V1H1L1/posplots.html)
[Comparison](https://ldas-jobs.ligo-wa.caltech.edu/~nv.krishnendu/LSC/lalinference/O3/s190814bv_inj/siqm_master_comparison_tightprior/) | :white_check_mark: |
## Notes on the codes
* There seems to be an inconsistency between naming the variables with lower and upper case: "dquadmona" vs "dQuadMon1". The post-processing scripts have lower case, while the sampling scripts have upper case. `Clarify with Krishnendu`.
* Newline characters missing at the end of Lines 237,245,270,278,309 and 317 in [diff.patch](https://git.ligo.org/nv.krishnendu/lalsuite/commit/7ec7378129a05b343dcf5d088f97426e41321949). `Action Item`
* The sampling is generating a lot of text; which will probably overload the log files. We should remove the print statements from the `LALInferenceCheckVariable` conditions within `lalinference/src/LALInferenceTemplate.c`. `Action Item`
* Changes to cbcBayesPPAnalysis code postponed for the time being
## Additional code change
* Based on the following [merge-request](https://git.ligo.org/lscsoft/lalsuite/-/merge_requests/1266), involving the deep copying of LALpars when caching waveforms.
* Relevant commit: https://git.ligo.org/nv.krishnendu/lalsuite/-/commit/2ecaaf92993deb667060570718d311b0a958de32