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

Various fixes to EventManagers #4639

Merged
merged 7 commits into from
Feb 20, 2024

Conversation

titodalcanton
Copy link
Contributor

@titodalcanton titodalcanton commented Feb 16, 2024

This addresses, at least in part, #4416 and #4632.

It only affects the HDF5 output of pycbc_multi_inspiral, specifically:

  • The single-detector snr_* datasets are renamed to just snr. This will break code downstream of pycbc_multi_inspiral, which will have to be modified (probably in a later PR though).
  • Per-detector gating information is now stored correctly.
  • The template_duration dataset is removed, the idea being that template parameters should always be taken from the bank file instead. Note that a few plotting codes in the all-sky offline search workflow do need template_duration to be in the trigger files. I think the main reason for this is that not every bank generation code adds the template duration dataset when generating the bank, so historically this has been done in pycbc_inspiral instead. I added a comment to explain this next to the relevant code in EventManager. I think this is not ideal, so we should try to avoid this in PyGRB workflows.

I have broken down the changes into small and logically-meaningful commits, hopefully this makes review easier and faster.

@titodalcanton titodalcanton added the PyGRB PyGRB development label Feb 16, 2024
@titodalcanton titodalcanton force-pushed the eventmgrcoh_fixes branch 4 times, most recently from 0a5edb9 to e076926 Compare February 19, 2024 16:05
@titodalcanton
Copy link
Contributor Author

It seems a few things in the all-sky coinc offline workflow actually depend on template_duration being in the output of pycbc_inspiral (I count at least plot_singles_timefreq, page_foreground, page_snglinfo, page_coincinfo). So I will revert that change in order to keep this PR focused on multi_inspiral.

Copy link
Contributor

@jakeb245 jakeb245 left a comment

Choose a reason for hiding this comment

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

Looks good to me. It will break things downstream in the PyGRB workflow, but it should be easy to fix and will simplify the code.

@titodalcanton titodalcanton merged commit 232a4ce into gwastro:master Feb 20, 2024
33 checks passed
@titodalcanton titodalcanton deleted the eventmgrcoh_fixes branch February 20, 2024 17:34
bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Mar 4, 2024
* Rename "snr_<detector>" datasets to just "snr"

* Do not store template duration in trigger files

* Cleanup and better document `write_events()` method

* Fix how gating info is written

* Fix imports

* Detriplicate code

* Codeclimate
lpathak97 pushed a commit to lpathak97/pycbc that referenced this pull request Mar 13, 2024
* Rename "snr_<detector>" datasets to just "snr"

* Do not store template duration in trigger files

* Cleanup and better document `write_events()` method

* Fix how gating info is written

* Fix imports

* Detriplicate code

* Codeclimate
acorreia61201 pushed a commit to acorreia61201/pycbc that referenced this pull request Apr 4, 2024
* Rename "snr_<detector>" datasets to just "snr"

* Do not store template duration in trigger files

* Cleanup and better document `write_events()` method

* Fix how gating info is written

* Fix imports

* Detriplicate code

* Codeclimate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PyGRB PyGRB development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants