Mass-gap-review.md

@@ -58,28 +58,40 @@ This outputs (p_NS, p_EMB, p_massgap)
 | M. Coughlin | ongoing | ligo.em-bright v1.1.0 |        :x:    |
 | S. Antier   | ongoing | ligo.em-bright v1.1.0 |        :x:    |
 
-##Comments
-
-- The code is running both from Coughlin and Antier condor https://git.ligo.org/emfollow/em-properties/em-bright/-/wikis/Mass-gap-review
-- clf_kwargs = {'n_estimators':50, 'criterion':'gini', 'max_depth': 20, 'max_features': None, 'min_samples_leaf': 5, 'min_samples_split': 20} is consistent with "https://git.ligo.org/emfollow/em-properties/mass_gap/-/blob/main/mass_gap_grid_search.ipynb" parameters grid search
-- Installation is very easy, thanks
-- Lack of understanding of conf.ini :
---> executables : two times the same time, can you clarify ?
---> lack of understanding and comments in config_ini parameters
---> What is the definition of the snr here ? 
---> Threshold of cfar : does it correspond to the FAR threshold for public alerts in O4 ? does it follow LL group definition ?
---> "weignts": distance : how the distance is calculated here by the NN ?
-
-- Be careful on personal repositories present in the code
+## Review Comments
+
+### What we checked:
+- The code is running for both Coughlin and Antier on condor thanks to the instructions (Installation is very easy, thanks): https://git.ligo.org/emfollow/em-properties/em-bright/-/wikis/Mass-gap-review
+- clf_kwargs = {'n_estimators':50, 'criterion':'gini', 'max_depth': 20, 'max_features': None, 'min_samples_leaf': 5, 'min_samples_split': 20} is consistent with "https://git.ligo.org/emfollow/em-properties/mass_gap/-/blob/main/mass_gap_grid_search.ipynb" parameters based on the hyper-parameter tuning grid search
+
+### Possible improvements:
+- conf.ini could benefit from more comments (some examples are below):
+--> executables : two times they are repeated, can you clarify ?
+--> lack of comments in config_ini parameters (for example, is SNR the network SNR?)
+--> Threshold of cfar : does it correspond to the FAR threshold for public alerts in O4 ? does it follow LL group definition ? Where does it come from?
+--> "weights": distance : briefly, how is the distance is calculated here by the NN ?
+
+- We should make sure to put under version control any data in personal repositories referenced in the code (as well as point to em-properties main):
 --> to run the classifier output_dir -i /home/sushant.sharma-chaudhary/O2-HL-rates-injections
 or https://git.ligo.org/emfollow/em-properties/em-bright/-/merge_requests/41/diffs#0064bf8c2663d62fc40433c07c51bf36875714d1_162_192
 lin 49 PACKAGE_DATA_LINKS["MASS_GAP.pickle"] = 'https://git.ligo.org/sushant.sharma-chaudhary/em-bright-gp/-/raw/massgap/ligo/em_bright/data/MASS_GAP.pickle'
 
-- in the command em_bright_dag_writer
---> -- mass-gap option, what does it refers to ?
---> Layer   em_bright_extract       39        1      --> where 39 comes to ? Arbitrary ?
+- in the command em_bright_dag_writer:
+--> -- mass-gap option, maybe add a docstring?
+--> Layer   em_bright_extract       39        1      --> where does the 39 come from ? Arbitrary ?
+
+- m1 threshold (120 solar masses) discussed in https://git.ligo.org/emfollow/em-properties/mass_gap/-/blob/main/mass_gap_grid_search.ipynb
+--> Where does this 120 come from exactly? Is this equivalent to setting the mass gap probability to 0 for such high template masses?
+--> Given the excess of confusion around the 0.4 threshold, perhaps better would be to carve out that prediction region and set to some other value that basically say "we don't know"? Seems like confusion will be high.
+--> At longer term, revisiting the injection set choices could be appropriate (with the astrophysical set from R+P perhaps, which focuses on lower mass objects).
+
+- SLy_param_sweep_mass_gap.png
+--> Just to confirm, this is running the trained classifier (on gstlal only) and sweeping m1, m2, and the spins?
+--> Maybe to understand striping better, could just zero out the parameter space for the 3 most relevant scenarios, i.e.  m1 is in mass gap (but not m2), m1 and m2 are in mass gap and m2 is in mass gap (but not m1) to understand more the strips
+--> Similarly, might plot the ratio between predictions for the high spinning and low spin case to see clearly how spin enters the predictions.
+
+ - https://git.ligo.org/emfollow/em-properties/mass_gap/-/blob/main/mass_gap_train.ipynb
+--> The 0.4 bump (which we mentioned before) in the out[9] figure is weird. Maybe the spin study above will be useful.
 
-- 120 m1 threshold discussed in https://git.ligo.org/emfollow/em-properties/mass_gap/-/blob/main/mass_gap_grid_search.ipynb
---> What is the value to set-up this threshold so high while astronomers are interesting for prompt observation in EM_bright objets and with mass-gap ? so 1< m2 < 3 ?
---> One suggestion would be to set a status as unknown when the rapid estimates clearly are deficient to avoid values that does not make sense.
---> For mid terme, one useful study would be to train the NN only with this interesting sub-set of injections ( 1< m2 < 3)
+- https://git.ligo.org/emfollow/em-properties/em-bright/-/merge_requests/41/diffs#16f7de7cdc8de9da5a39214ab57e1c93fb081629
+--> We read through the code, and it does look good, but again would benefit from additional comments around line #205 and elsewhere.