Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add PyGRB Exclusion Distance Table Executable #4756
Add PyGRB Exclusion Distance Table Executable #4756
Changes from 14 commits
56f7fed
b233a93
7edb1df
8d04509
0777fcb
2325d0e
48d4263
ae51658
87a0933
5f34113
199ed48
0bf9ea8
833ee22
7a3fdde
b200710
6852c27
50291df
44a64f4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 must provide
opts.injection_set_name
ifopts.exclusion_dist_output_file
and/oropts.found_missed_file
are provided.Looking at this made me wonder about something I had not caught in the past: does it even make sense to have
opts.found_missed_file
andopts.onsource_file
as optional? I.e., ifdo_injections
and/oronsource_file
are False, does the code do something meaningful (and not crash)? Otherwise we can promote those options to required and remove the ifs ondo_injections
andonsource_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.
The injections part of the code uses results from the onsource analysis, so I'd say it wouldn't hurt anything to make both files required, but there could be cases I'm not considering.
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.
Should we make
--trial-name
required inpycbc_pygrb_efficiency
to prevent this? This way we have a check in place here for the standalone script, and a way to dodge the error in the 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.
Now that I looked better,
pycbc_pygrb_efficiency
must check thatopts.trial_name
is not None whenexclusion_dist_output_file
is not None.