Skip to content

Changes

Page history
Update Home authored by NV Krishnendu's avatar NV Krishnendu
Show whitespace changes
Inline Side-by-side
Home.md
View page @ d06f1850
......@@ -54,13 +54,15 @@ To check the readiness of this pipeline for the analysis of O3 events.
# Detail of the modifications
Git hash of the branch to be reviewed: 5157eea61128250568beca71edbc18c165a4a45e
- Git hash of the branch to be reviewed: 5157eea61128250568beca71edbc18c165a4a45e
> This for the first review meeting on 24/05/2019, 07/06/2019 and 21/06/2019. The modifications suggested by the reviewers are in this commit:
| Files modified |Lines changed| Short explanation | Person Responsible | Reviewer Response | Addressing Issue (if any) |
| -------- | --- | ----- | -------- | -------- | -------- |
|[LALInference.c](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInference.c) | [L2322](https://git.ligo.org/nv.krishnendu/lalsuite/blob/eb5f2658c960ad689625c66c7df9e9373a4a27d6/lalinference/src/LALInference.c#L2322) to [L2327](https://git.ligo.org/nv.krishnendu/lalsuite/blob/eb5f2658c960ad689625c66c7df9e9373a4a27d6/lalinference/src/LALInference.c#L2327)| Function to calculate dQuadMonS and dQuadMonA from dQaudMon1 and dQuadMon2|Abhirup & Anuradha G. |**Gupta**: looks good. <br> **Abhirup**: looks good. <br> General clarifications: <br>1. Out of (dQuadMonS, dQuadMonA,dQaudMon1,dQuadMon2) only two are independent; what are the problems of sampling over just two and constructing the others in post-processing, rather than keeping all four as sampling parameters [**Answer**: there are certain combinations of these quantities that are easier to sample over to yield better constraints; provide options to either sample on (dQaudMon1,dQuadMon2) or (dQuadMonS, dQuadMonA) depending on user.] <br>2. And if all 4 parameters need to be kept in the code, is there a way of making it explicit to the user to choose at most 2 of the 4 parameters for sampling [**Action Item**: Error message along the lines of tidal parameters to make sure that sampling happens over at most 2 parameters]. <br> **Krishnendu**: <br> 1. These comments are addressed. The current structure provides options to either sample on (dQaudMon1,dQuadMon2) or (dQuadMonS, dQuadMonA). If more than two parameters are passed, sampling will be done on the default option of sampling on (dQaudMon1,dQuadMon2) with a warning message.| No further changes made |
|[LALInference.c](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInference.c) | [L2322](https://git.ligo.org/nv.krishnendu/lalsuite/blob/eb5f2658c960ad689625c66c7df9e9373a4a27d6/lalinference/src/LALInference.c#L2322) to [L2327](https://git.ligo.org/nv.krishnendu/lalsuite/blob/eb5f2658c960ad689625c66c7df9e9373a4a27d6/lalinference/src/LALInference.c#L2327)| Function to calculate dQuadMonS and dQuadMonA from dQaudMon1 and dQuadMon2|Abhirup & Anuradha G. |**Gupta**: looks good. <br> **Abhirup**: looks good. <br> General clarifications: <br>1. Out of (dQuadMonS, dQuadMonA,dQaudMon1,dQuadMon2) only two are independent; what are the problems of sampling over just two and constructing the others in post-processing, rather than keeping all four as sampling parameters [**Answer**: there are certain combinations of these quantities that are easier to sample over to yield better constraints; provide options to either sample on (dQaudMon1,dQuadMon2) or (dQuadMonS, dQuadMonA) depending on user.] <br>2. And if all 4 parameters need to be kept in the code, is there a way of making it explicit to the user to choose at most 2 of the 4 parameters for sampling [**Action Item**: Error message along the lines of tidal parameters to make sure that sampling happens over at most 2 parameters]. | <br> **Krishnendu**: <br> 1. These comments are addressed. The current structure provides options to either sample on (dQaudMon1,dQuadMon2) or (dQuadMonS, dQuadMonA). If more than two parameters are passed, sampling will be done on the default option of sampling on (dQaudMon1,dQuadMon2) with a warning message.|
|[LALInference.h](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInference.h) | [L882](https://git.ligo.org/nv.krishnendu/lalsuite/blob/eb5f2658c960ad689625c66c7df9e9373a4a27d6/lalinference/src/LALInference.h#L882) to [L883](https://git.ligo.org/nv.krishnendu/lalsuite/blob/eb5f2658c960ad689625c66c7df9e9373a4a27d6/lalinference/src/LALInference.h#L883)| Declaring above function LALInferencedQuadMonSdQuadMonA in header file|Abhirup & Anuradha G. |**Gupta**: looks good. <br> **Abhirup**: looks good.| No further changes made |
|[LALInferenceTemplate.c](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceTemplate.c) | [L74](https://git.ligo.org/nv.krishnendu/lalsuite/blob/eb5f2658c960ad689625c66c7df9e9373a4a27d6/lalinference/src/LALInferenceTemplate.c#L74) to [L76](https://git.ligo.org/nv.krishnendu/lalsuite/blob/eb5f2658c960ad689625c66c7df9e9373a4a27d6/lalinference/src/LALInferenceTemplate.c#L76)| Adding extra params for sampling |Abhirup & Anuradha G. |**Gupta**: looks good. <br> **Abhirup**: looks good.| This is removed now as the spin-induced parameters are not part of testing-GR params |
|[LALInferenceTemplate.c](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceTemplate.c) | [L74](https://git.ligo.org/nv.krishnendu/lalsuite/blob/eb5f2658c960ad689625c66c7df9e9373a4a27d6/lalinference/src/LALInferenceTemplate.c#L74) to [L76](https://git.ligo.org/nv.krishnendu/lalsuite/blob/eb5f2658c960ad689625c66c7df9e9373a4a27d6/lalinference/src/LALInferenceTemplate.c#L76)| Adding extra params for sampling |Abhirup & Anuradha G. |**Gupta**: looks good. <br> **Abhirup**: looks good.| No further changes made. This is removed now as the spin-induced parameters are not part of testing-GR params |
|[LALInferenceTemplate.c](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceTemplate.c) |[L358](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceTemplate.c#L358) to [L408](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceTemplate.c#L408) [L857](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceTemplate.c#L857) to [L905](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceTemplate.c#L905) [L1461](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceTemplate.c#L1461) to [L1512](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceTemplate.c#L1512)|Following ways of sampling are introduced. <br>1. If one parameter is passed among the four dQuadMon parameters, one would be able to sample on dQuadMon1, dQuadMon2, dQuadMonS or dQuadMonA individually. <br> 2. For the two parameter case, either dQuadMon1 and dQuadMon2 or dQuadMonS and dQuadMonA can be sampled. If any other combination is passed, sampling will be done on dQuadMon1 and dQuadMon2 (dafault option) with a warning message. <br> 3. If more than two prameters are passed, sampling will be done on dQuadMon1 and dQuadMon2 (dafault option) with a warning message. |Abhirup & Anuradha G. |**Gupta**:Same set of lines in three palces? Also, in your implimentation you assumed \kappa_a to be zero. That means you assumed both the stars to have same \kappa. How this assumption biases the results, especially on real data? <br> **Abhirup**: <br> The three sets of lines are for definitions inside 3 separate functions: LALInferenceROQWrapperForXLALSimInspiralChooseFDWaveformSequence, LALInferenceTemplateXLALSimInspiralChooseWaveform and LALInferenceTemplateXLALSimInspiralChooseWaveformPhaseInterpolated. <br> Wouldn't it be easier to allow sampling over dQuadMonA and if required fix it to injection or any other value from the config.ini using --pin-params = [dQuadMonA] or --fix-dQuadMonA [**ACTION ITEM**: Create a more generalised framework where any (and at most) two parameters can be sampled over; there will be an error if more than two parameters are specified to be sampled over; parameters can be held fixed from the config.ini if required] <br> **Krishnendu**: <br> 1. This has been implimented. <br> 2. We also did test runs for this for a high mass system and the run directory is here: LHO /home/nv.krishnendu/review_test_runs/Modified_template/. The corresponding .err and .out files contain error messages if there any. Right now, we are also printing the values of these parameters and these may be removed when we merge to the review branch.| The earlier functions (at three places) re-defined as the sampling parameter choices are revised now. |
|[LALInferenceInitCBC.c](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceInitCBC.c) | [L2172](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceInitCBC.c#L2172) to [L2175](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceInitCBC.c#L2175) [L2210](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceInitCBC.c#L2210) to [L2250](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceInitCBC.c#L2250)| Sampling options and warning (error) messages added inside the function `LALInferenceInitNonGRParams`| Abhirup & Anuradha G.| **Gupta**: L2172-L2179: Is prior till 20 large enough? L2214-L2218: looks good. <br> **Abhirup**: <br> 1. the current prior range excludes the space for boson stars <br>2. tmpVal = 0, is at the default prior boundary <br> Perhaps making the prior more symmetric around zero addresses point 1 and keeps the starting value close to the middle of the prior. <br> **Krishnnendu**: <br> 1. The prior range has been increased to [-200, 200], but tmpVal = 0 is kept as it is.| The spin-induced parameters are removed from `LALInferenceInitNonGRParams(` and changed the following lines. <br> These params are added in `LALInferenceModel *LALInferenceInitCBCModel(LALInferenceRunState *state)` at [L793](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceInitCBC.c#L793), the prior ranges are fixed at line [L880](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceInitCBC.c#L880), following tidal parameter lines from [L1410](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceInitCBC.c#L1410) to [L1425](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/src/LALInferenceInitCBC.c#L1425) is changed.<br> As from the current changes, for the one parameter case any of the four parameters cane be sampled (one has to fix the other parameter using fix-parameters). <br> For the two parameter case, either {dQuadMon1, dQuadMon2} or {dQuadMonS, dQuadMonA}. <br> If one passes more than two parameters, the run will end with a exist message|
|[cbcBayesCompPos.py](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/python/cbcBayesCompPos.py) | [L59](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/python/cbcBayesCompPos.py#L59) and [L71](https://git.ligo.org/nv.krishnendu/lalsuite/blob/Ks_21March2019/lalinference/python/cbcBayesCompPos.py#L71)| `paramNameLatexMap` (convert parameter names to Latex), we define dQuadMon parameter as \delta\kappa <br> added to `clTableParams` (this is to add to the table of confidence levels)| Abhirup & Anuradha G. |**Gupta**: Just \kappa_s is defined here, why not \kappa_1, \kappa_2, \kappa_a as in bayespputils.py? <br> **Abhirup**: Same question as Anuradha. <br> **Krishnnendu**: <br> 1. All the four parameters are added. | -------- |
......