-
Notifications
You must be signed in to change notification settings - Fork 356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handling vetoes and timeslides in pygrb mini followups #4941
Conversation
…ng the correct timeslide when following triggers/injections
…s to extract_trig_properties
logging.info('Processing event: %s', num_event+1) | ||
gps_time = fp['GPS time'][num_event] | ||
gps_time = gps_time.astype(float) | ||
tags = args.tags + [str(num_event+1)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting these to match the row numbering we see in the loudest offsource event / quite injections tables.
else: | ||
for i_ifo, ifo in enumerate(ifos): | ||
time_shift = fp[ifo+' time shift (s)'][num_event] | ||
ifo_time = gps_time + time_shift |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from - to + is intentional here. This was sliding the wrong way!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes on the pygrb_postprocessing_utils.py
as right now would break pycbc_pygrb_efficiency
, pycbc_pygrb_page_tables
, and pycbc_pygrb_plot_stats_distribution
. So these three files need some adjustments
|
||
return trig_time, trig_snr, trig_bestnr | ||
return found_trigs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pycbc_pygrb_efficiency
, pycbc_pygrb_page_tables
, and pycbc_pygrb_plot_stats_distribution
are using this function and need three outputs not to break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, in the development version these scripts will call extract_trig_properties
to get these quantities. But, I am breaking down the many changes in multiple PRs so that the diffs may be parsed with reasonable effort: one big PR would be complicated to handle for a reviewer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with breaking it down, but approving this PR before any of the others would find difficult to run workflows without issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was true for quite a while until PR #4909, when PyCBC allowed to generate a (working) PyGRB workflow, but without vetoes. As for previous "PR series" that took us to that point, the idea is that we are enabling a new big feature and, by breaking changes down, accepting that the intermediate states of PyCBC between PR #4909 and the completion of this PR review work will break PyGRB again. At that point we will have a PyCBC where PyGRB workflows are fully working and with vetoes. It will then be the right moment to have a new CI/CD test with a small PyGRB search and hence stop this modus operandi. At the moment PyGRB is already broken on master :-)
|
||
# Sort the triggers into each slide | ||
sorted_trigs = sort_trigs(trial_dict, trigs, slide_dict, seg_dict) | ||
logger.info("Triggers sorted.") | ||
n_surviving_trigs = sum([len(i) for i in sorted_trigs.values()]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave the log info here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this because the logging info below is more informative, while is still telling the user the triggers were sorted.
"""Extract and store as dictionaries time, SNR, and BestNR of | ||
time-slid triggers""" | ||
def extract_trig_properties(trial_dict, trigs, slide_dict, seg_dict, keys): | ||
"""Extract and store as dictionaries specific keys of time-slid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the change of name of the function. But since this and its precursor do two different things, shouldn't we keep them both? (see also comment on the output)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the answer above. The call to the old function will be removed throughout PyGRB.
trig_bestnr[slide_id] = reweightedsnr_cut( | ||
trigs['network/reweighted_snr'][indices], | ||
opts.newsnr_threshold) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we using this anymore? If just one of the functions is kept, maybe one could return the found_trigs
as well as the bestNR
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user will get the 'network/reweighted_snr'
by calling extract_trig_properties
with 'network/reweighted_snr'
among the keys
. So this function is not returning the found_trigs
dictionary with the keys
requested (and 'network/reweighted_snr'
may be one of these).
Yes, correct, and the changes will be part of the next PRs in this series. If I were to produce a single PR, it would be too big to parse by the reviewer(s). |
* Use veto and segments files in pycbc_pygrb_minifollowups; enforce using the correct timeslide when following triggers/injections * Added note about pycbc_pygrb_minifollowups in pycbc_pygrb_page_tables * Updated construct_trials and generalized extract_basic_trig_properties to extract_trig_properties * Edited var name to something more meaningful * More accurate docstring * Aesthetics and missing import * Simplified pycbc_pygrb_minifollowups and fixed a sign! * Removed empty line * f-string and bare except
This PR is the third in the series started in PR #4929.
Standard information about the request (and the following ones that will be linked to this)
This is a: a new feature enabling veto definer file usage in PyGRB. Utilities and scripts in results production are being streamlined along the way.
This change affects: PyGRB
This change changes: result presentation / plotting and scientific output.
If this change breaks the standard automated test running
--help
for PyGRB plotting scripts, I will add some workarounds to avoid this. If needed, these will likely be empty functions: the plotting scripts will be progressively renovated in the whole series of PRs.Motivation
Now that the workflow generator passes around the veto definer file, the mini followups need to handle it use it. In checking
pycbc_pygrb_minifollowups
I noticed that it is better to enforce the correct timesmlide is being plotted.Contents
pycbc_pygrb_minifollowups
and the jobs it prepares.construct_trials
was improved and the utilityextract_basic_trig_properties
was generalized toextract_trig_properties
in which the user specifies what datasets need to be extracted from the trigger file.Testing performed
The totality of the changes that will be broken down in multiple PRs was tested on GRB 170817A data by producing a full results webpage (see here).