-
Notifications
You must be signed in to change notification settings - Fork 348
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
Improved pygrb missed/quiet injection tables job handler #4877
Conversation
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.
Request to consider using File objects to avoid potential failure modes. However, this will not hit the potential failure modes as written, so happy to not hold things up.
pycbc/workflow/grb_utils.py
Outdated
node = PlotExecutable(workflow.cp, exec_name, | ||
ifos=workflow.ifos, out_dir=out_dir, | ||
tags=tags+extra_tags).create_node() | ||
# Pass the bank-file | ||
node.add_input_opt('--bank-file', resolve_url_to_file(bank_file)) |
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.
Same comment here with bank_file
, pass File
objects around, not URLs. There are potential failure modes with file handling here if resolve_url_to_file
doesn't realise it is the same file every time.
pycbc/workflow/grb_utils.py
Outdated
# Pass the bank-file | ||
node.add_input_opt('--bank-file', resolve_url_to_file(bank_file)) | ||
# Offsource input file (or equivalently trigger file for injections) | ||
offsource_file = resolve_url_to_file(off_file) |
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.
And here
pycbc/workflow/grb_utils.py
Outdated
'sngl-snr-threshold', 'null-grad-thresh', 'null-grad-val']: | ||
if workflow.cp.has_option('workflow', opt): | ||
node.add_opt('--'+opt, workflow.cp.get('workflow', opt)) | ||
seg_filelist = FileList([resolve_url_to_file(sf) for sf in seg_files]) |
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.
Here again
pycbc/workflow/grb_utils.py
Outdated
fm_file = configparser_value_to_file(workflow.cp, | ||
'injections-'+inj_set, | ||
'found-missed-file') | ||
fm_file = resolve_url_to_file(inj_file) |
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.
Why would this not already be a file object here???
pycbc/workflow/grb_utils.py
Outdated
if on_file is not None: | ||
src_type = 'onsource-trig' | ||
# Onsource input file (passed as File instance) | ||
onsource_file = resolve_url_to_file(on_file) |
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.
Here too.
@spxiwh, I followed up on your comments, thanks. I reran the post-processing portion of the run and successfully produced a new webpage which looks like the original one. |
This PR improves how jobs are added to the PyGRB workflow to create tables with "quiet-found" and "missed-found injections". The function is (hopefully) cleaner and it handles its inputs more appropriately.
Standard information about the request
This is an improvement of an existing feature.
This change affects: PyGRB
This change changes: result presentation
Links to any issues or associated PRs
This change is the 4th PR of a series started with #4872. It should be the last one for
grb_utils.py
.Testing performed
As for the previous 3 PRs, a complete webpage example is available here, but the full webpage will be reproducible only once all workflow changes are on master.