Skip to content
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

Merged
merged 18 commits into from
May 29, 2024

Conversation

jakeb245
Copy link
Contributor

@jakeb245 jakeb245 commented May 20, 2024

Standard information about the request

This PR adds an executable to create a static table for PyGRB exclusion distances and updates pycbc_pygrb_efficiency to output the necessary data to a JSON file.

This is a: new feature

This change affects: PyGRB

This change changes: result presentation

Motivation

PyGRB result webpages currently write the exclusion distances in the plot captions. A table containing this information would look nicer.

Contents

  • Updates pycbc_pygrb_efficiency to write a JSON file with the exclusion distances
  • Adds pycbc_pygrb_exclusion_distance_table which takes the JSON output files and outputs a result table.

Testing performed

An example of the resulting table can be seen here. This is a quick example generated on the command line. For a final result, the table will be added to the results webpage within the workflow generator (to be done in a later PR).

Additional notes

  • The author of this pull request confirms they will adhere to the code of conduct

@jakeb245 jakeb245 added the PyGRB PyGRB development label May 20, 2024
@jakeb245 jakeb245 self-assigned this May 20, 2024
@jakeb245 jakeb245 requested a review from pannarale May 20, 2024 19:57
bin/pygrb/pycbc_pygrb_efficiency Outdated Show resolved Hide resolved
bin/pygrb/pycbc_pygrb_exclusion_dist_table Outdated Show resolved Hide resolved
bin/pygrb/pycbc_pygrb_exclusion_dist_table Outdated Show resolved Hide resolved
bin/pygrb/pycbc_pygrb_exclusion_dist_table Show resolved Hide resolved
Comment on lines +56 to +57
if len(trials) == 0:
raise ValueError("No trials found in input files.")
Copy link
Contributor

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 in pycbc_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.

Copy link
Contributor

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 that opts.trial_name is not None when exclusion_dist_output_file is not None.

bin/pygrb/pycbc_pygrb_efficiency Show resolved Hide resolved
# Also include injection set name and trial name
if opts.exclusion_dist_output_file:
excl_dist_dict = {}
excl_dist_dict['inj_set'] = inj_set_name
Copy link
Contributor

@pannarale pannarale May 21, 2024

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 if opts.exclusion_dist_output_file and/or opts.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 and opts.onsource_file as optional? I.e., if do_injections and/or onsource_file are False, does the code do something meaningful (and not crash)? Otherwise we can promote those options to required and remove the ifs on do_injections andonsource_file.

Copy link
Contributor Author

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.

@pannarale pannarale merged commit 76f2df5 into gwastro:master May 29, 2024
33 checks passed
@pannarale pannarale mentioned this pull request Jun 1, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PyGRB PyGRB development
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants