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.

Sign up for GitHub

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

Jump to bottom

Automatically make files for comprehensive offline gracedb uploads #4438

Closed
wants to merge 50 commits into from
Closed

Automatically make files for comprehensive offline gracedb uploads #4438

wants to merge 50 commits into from

Conversation

GarethCabournDavies
Copy link
Contributor

@GarethCabournDavies GarethCabournDavies commented Jul 14, 2023 •
edited
Loading

In order to avoid manually making skymaps for the data release (as in O3), we want to include files in the offline uploads as standard.

This includes:

  • the XML being uploaded including the PSD and SNR timeseries
  • an HDF file being uploaded, alongside the PSD (as is done for live)
  • a plot of the SNR timeseries and PSDs for each detector (active or not)
  • The fits file and a skymap plot, made using information which would be lost if it relied on GraceDB information

NB - edits to this description are in italic text

titodalcanton
titodalcanton reviewed Jul 24, 2023
View reviewed changes
pycbc/results/snr.py Outdated Show resolved Hide resolved
titodalcanton
titodalcanton reviewed Jul 24, 2023
View reviewed changes
pycbc/results/snr.py Outdated Show resolved Hide resolved
titodalcanton
titodalcanton reviewed Jul 24, 2023
View reviewed changes
pycbc/results/snr.py Outdated Show resolved Hide resolved
@GarethCabournDavies GarethCabournDavies force-pushed the snr_timeseries_gdb_upload branch 2 times, most recently from 1702923 to 679252d Compare August 9, 2023 13:58
@GarethCabournDavies
Copy link
Contributor Author

I'm struggling to get the xmlall to be transferred into the minifollowup at the moment, but have added skymap creation and plotting.

For using bayestar in a workflow, I've suggested allowing a set filename for output https://git.ligo.org/lscsoft/ligo.skymap/-/merge_requests/331

@GarethCabournDavies
Copy link
Contributor Author

Now trying a wrapper script workaround as bayestar doesnt look like it will be able to output individual filenames any time soon

Search will fail as we don't include ligo-make-skymap or bayestar-localize-coincs in pycbc. I will remove the changes from the config files before merging

@GarethCabournDavies
Copy link
Contributor Author

There is an error with run small search using pegasus + condor, which will be fixed once I revert the changes to the config files.

There are also a couple minor changes I plan to make to the single-event-upload a script, but I think this is ready to review otherwise.

GarethCabournDavies
GarethCabournDavies commented Aug 21, 2023
View reviewed changes
Copy link
Contributor Author

@GarethCabournDavies GarethCabournDavies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few comments from myself for description / fixing a couple of things

bin/all_sky_search/pycbc_make_bayestar_skymap Show resolved Hide resolved
bin/all_sky_search/pycbc_prepare_xml_for_gracedb Show resolved Hide resolved
pycbc/workflow/minifollowups.py Outdated Show resolved Hide resolved
pycbc/workflow/minifollowups.py Outdated Show resolved Hide resolved
pycbc/workflow/minifollowups.py Outdated Show resolved Hide resolved
pycbc/workflow/minifollowups.py Outdated Show resolved Hide resolved
pycbc/workflow/psd.py
return merge_psds(workflow, psd_files, ifo, out_dir, tags=tags)
else:
return psd_files[0]
return merge_psds(workflow, psd_files, ifo, out_dir, tags=tags)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always use merged psds, even if there is only one calculate_psd job

@GarethCabournDavies GarethCabournDavies changed the title Draft: Include SNR timeseries in offline gracedb uploads Include SNR timeseries in offline gracedb uploads Aug 21, 2023
@GarethCabournDavies
Copy link
Contributor Author

Example upload:

https://gracedb-playground.ligo.org/events/G1190222/view/

@GarethCabournDavies
Copy link
Contributor Author

poke @titodalcanton

titodalcanton
titodalcanton reviewed Oct 11, 2023
View reviewed changes
bin/all_sky_search/pycbc_prepare_xml_for_gracedb
for sngl in sngl_inspiral_table:
if sngl.event_id not in sngl_ids:
continue
sngl_inspiral_table_curr.append(sngl)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we including "subthreshold" detectors here? E.g. how would the single inspiral table for a L1-only trigger with observing-mode H1 look like?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not including subthreshold detectors here, this is inherited from the pycbc_page_foreground output, so does not have this info added. I'm not sure whether we'd want that though or not

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one from the github example run, and have uploaded to https://gracedb-playground.ligo.org/events/G1268054/view/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I suspect we do need the single inspiral entries to be present in order for BAYESTAR to use the subthreshold detectors, but let's keep things as they are for now. We can fix that with a later PR if needed.

GarethCabournDavies
GarethCabournDavies commented Oct 13, 2023
View reviewed changes
bin/all_sky_search/pycbc_upload_single_event_to_gracedb
@@ -0,0 +1,185 @@
#!/usr/bin/env python
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably say that this code would be run after the workflow has finished, and the box is opened.

We would use something like:

pycbc_upload_single_event_to_gracedb \
  --xml-file-for-upload \
    $upload_data_dir/H1L1V1-GENERATE_XML_FULL_DATA_UPLOAD_3-1186740100-3400.xml \
  --snr-timeseries-plot \
    $upload_data_dir/H1L1V1-GENERATE_XML_FULL_DATA_UPLOAD_3_SNR-1186740100-3400.png \
  --asd-plot \
    $upload_data_dir/H1L1V1-GENERATE_XML_FULL_DATA_UPLOAD_3_PSD-1186740100-3400.png \
  --skymap-plot \
    $upload_data_dir/H1L1V1-SKYMAP_PLOT_FULL_DATA_UPLOAD_3-1186740100-3400.png \
  --skymap-fits-file \
    $upload_data_dir/H1L1V1-BAYESTAR_FULL_DATA_UPLOAD_3-1186740100-3400.fits \
  --labels \
    GRACEDB_LABEL

With the upload number (3 here) being looped over and timestamp updated as appropriate

titodalcanton
titodalcanton reviewed Oct 13, 2023
View reviewed changes
bin/all_sky_search/pycbc_upload_single_event_to_gracedb Outdated
Comment on lines 34 to 35
logging.basicConfig(format='%(asctime)s:%(levelname)s : %(message)s',
level=logging.INFO)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use PyCBC's logging_init() function here.

titodalcanton
titodalcanton reviewed Oct 13, 2023
View reviewed changes
bin/all_sky_search/pycbc_upload_single_event_to_gracedb Outdated
labels = [l.upper() for l in args.labels]
allowed_labels = gracedb.allowed_labels

if not all([l in allowed_labels for l in labels]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not all([l in allowed_labels for l in labels]):
if set(labels) - set(allowed_labels):

titodalcanton
titodalcanton reviewed Oct 13, 2023
View reviewed changes
bin/all_sky_search/pycbc_upload_single_event_to_gracedb Outdated
Comment on lines 146 to 149
comment = "PyCBC ASD estimate from the time of event"
tag = "psd"
displayname='ASDs'
upload_file(args.asd_plot, displayname, comment, tag)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of variables here is a bit verbose, I suggest this:

Suggested change
comment = "PyCBC ASD estimate from the time of event"
tag = "psd"
displayname='ASDs'
upload_file(args.asd_plot, displayname, comment, tag)
upload_file(
args.asd_plot,
displayname='ASDs',
comment='PyCBC ASD estimate from the time of event',
tag='psd'
)

Copy link
Contributor

@titodalcanton titodalcanton Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment for the cases below of course.

titodalcanton
titodalcanton reviewed Oct 13, 2023
View reviewed changes
bin/minifollowups/pycbc_foreground_minifollowup Outdated
ifo_list = f.attrs['ifos'].split(' ')
for ifo in ifo_list:
times[ifo] = f['{}/{}/time'.format(file_val,ifo)][:][event_idx]
tids[ifo] = f['{}/{}/trigger_id'.format(file_val, ifo)][:][event_idx]
bank_ids = f['{}/template_id'.format(file_val)][:][event_idx]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bank_ids = f['{}/template_id'.format(file_val)][:][event_idx]
bank_ids = f[file_val]['template_id'][:][event_idx]

though you can remove quite a bit of verbosity with an auxiliary variable f_cat = f[file_val] around line 102.

titodalcanton
titodalcanton reviewed Oct 13, 2023
View reviewed changes
bin/minifollowups/pycbc_foreground_minifollowup Outdated
tids
)

sngl_tmplt_files, sngl_tmplt_plots = mini.make_single_template_plots(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the variable sngl_tmplt_files actually used below? If not, turn to an underscore.

@GarethCabournDavies GarethCabournDavies changed the title Include SNR timeseries in offline gracedb uploads Automatically make files for comprehensive offline gracedb uploads Oct 13, 2023
@GarethCabournDavies
Copy link
Contributor Author

Updated the PR name as the scope has widened somewhat

This was referenced Oct 16, 2023
Merged
Merged
@GarethCabournDavies
Copy link
Contributor Author

We are splitting this PR into two: #4534 and #4535, as the changes were getting too long-winded for a single review

@GarethCabournDavies GarethCabournDavies deleted the snr_timeseries_gdb_upload branch October 16, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@titodalcanton titodalcanton titodalcanton left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@GarethCabournDavies @titodalcanton