-
Notifications
You must be signed in to change notification settings - Fork 352
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
PyGRB now establishes the tc prior for injections at workflow time #4751
Conversation
…ycbc_make_offline_grb_workflow
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 have one comment, but this PR only touches GRB code, so feel free to merge this if you want to ignore it.
if tc_file_path not in config_urls: | ||
config_urls += [tc_file_path] | ||
config_urls = ', '.join([str(item) for item in config_urls]) | ||
wflow.cp.set("workflow-injections", |
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.
As a general comment, I'm not sure I like the method of "generate new file and then copy it around by editing the ConfigParser object". In my vision for the workflow module the ConfigParser object should be immutable (after directives and shortcuts have been resolved when it's read in).
I would prefer to pass any new files around.
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.
Yeah, I'm not fond of that either but I can't think of another solution because create_injections
does not admit specifying a prior like that from command line, as far as I understand. PyGRB has already been forced to edit the ConfigParser for years here, by the 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.
I'm happy using a File to store the prior, but I think it should be passed around the workflow as a File object, not by editing the ini file.
However, that's just my opinion, and I'm happy for you to set standards in the grb_utils
module, so approval is given if you want to merge as is.
…wastro#4751) * PyGRB now establishes the tc prior for injections at workflow time * Makes sure PyGRB injections are only in the offsource * Created the method generate_tc_prior and cleaned up indentations in pycbc_make_offline_grb_workflow * Consistent returns from generate_triggered_segment
Standard information about the request
This is a: new feature for consistency with injections times within PyGRB.
This change affects: PyGRB.
This change changes: the workflow generator to avoid "wasting" injections.
This change: was tested in a full PyGRB workflow run.
Motivation
When setting up the coherent search, the user must specify a max-duration and a min-duration of the search and provides a trigger time (and a time window for the possible search which generally extends for max-duration seconds before and after the trigger time). The workflow generator then determines how to place the analysis time around the trigger time to maximise the coincident time with the detectors in the network specified by the user. It is only at this point that it becomes clear when in time the injections can be allowed to happen: if a prior for the time of the injections were specified from the start, based on the time window around the trigger time set up by the user for inspection, a(n unknown) fraction of the injections would be wasted.
Contents
This PR allows the user to ask the workflow generator to write up the prior for the time of the injections, after the analysis time is optimally determined. The prior is written to a file which is passed to all injection sets.
Testing performed
Plot from a workflow with 500 injections which precedes this PR: .
The injections were allowed to occur at any time the user was happy to consider for the search (roughly [-max-duration, +max-duration] around the trigger time) and the plot is showing all recovered injections up to the cut on reweighted SNR.
Here is the impact on missing injections:
Lots of nearby injections were missed (black crossed) because of when they happened.
Corresponding plots after the feature added in this PR, with a "stress-test" of 2500 injections:
(Please ignore the change in the color bar, which was part of a separate fix: the main point is that nearby injections are no longer black crosses, i.e., missed.)