-
Notifications
You must be signed in to change notification settings - Fork 355
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
Preparing for veto file handling in PyGRB #4929
Conversation
…b_postprocessing_utils.py
@spxiwh, @titodalcanton I added both of you as reviewers since there will be a combination of workflow and "pure PyGRB" content in this chain of PRs. |
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 only touches PyGRB code, so if it does what you want, then no issues from me. I leave a couple of optional comments. Should also wait to hear approval from Tito.
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.
Looks good apart from several minor comments and questions.
vetoes.coalesce() | ||
for ifo in ifos: | ||
for v in vetoes[ifo]: | ||
v_span = v[1] - v[0] |
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 think you can just do abs(v)
here, though I am not sure it is more readable :)
coinc_snr = numpy.array([]) | ||
if 'network/coherent_snr' in trigs_or_injs.keys() and \ | ||
'network/null_snr' in trigs_or_injs.keys(): | ||
coh_snr_sq = numpy.square(trigs_or_injs['network/coherent_snr'][:]) | ||
null_snr_sq = numpy.square(trigs_or_injs['network/null_snr'][:]) | ||
coinc_snr = numpy.sqrt(coh_snr_sq + null_snr_sq) |
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.
Not necessarily a request for change as this is probably coupled with downstream code, but would it make more sense to return either None or an array of NaNs, rather than an empty array?
* Missing empty line * grb_utils.py preparation to handle veto-files and segment files * pycbc_make_offline_grb_workflow update based on grb_utils.py upgrade * Updated hacky pygrb stuff in jobsetup.py * First batch of updates to handle the VDF in PyGRB and streamline pygrb_postprocessing_utils.py * Removing build_veto_filelist from pycbc_pygrb_minifollowups * Better variable naming and string search in jobsetup.py * Removing redundant variables * Corrected call to segments plots * Better argparse syntax * No f-string in logging * assert instead of if
This is the first PR of a series that will enable PyGRB to handle an xml veto file when producing final results (plots, p-values, tables, etc.)
Standard information about the request (and the following ones that will be linked to this)
This is a: a new feature in PyGRB. As an aside, some utilities and scripts will be streamlined.
This change affects: PyGRB
This change changes: result presentation / plotting and scientific output.
This change will probably break current functionalities at the moment. If the standard automated test running
--help
on all scripts fails on 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 PRs following this one.Motivation
PyGRB in its current version is not using data vetoes.
Contents
grb_utils.py
and the veto file is handled in parallel with the segments files throughout the functions (as noted in Combine PyGRB segments files #4887 the segments files will need to become a single json file eventually).jobsetup.py
.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).