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

Enabling single ifo runs in PyGRB workflows #5048

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

sebastiangomezlopez
Copy link
Contributor

Standard information about the request

This is a: bug fix

This change affects: PyGRB

This change changes: scientific output

Disclaimer: This PR has not been fully tested yet. DO NOT merge

The proposed changes in this Pull request include:

  • An expansion of the single IFO case at the pycbc_make_sky_grid level.
  • minor fixes to pycbc_multi_inspiral that were preventing the creation of "trivial" time_delay dictionaries when slide_num=0, and slide_id related lists and dictionaries only contain the 0 element.

Motivation

Contents

Links to any issues or associated PRs

Testing performed

Additional notes

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

@pannarale
Copy link
Contributor

I would keep the changes to the 2 files in 2 distinct PRs. The changes to pycbc_make_skygrid are solid and this PR could be renamed appropriately. Changes to pycbc_multi_inspiral require more thought, in my opinion.

@titodalcanton
Copy link
Contributor

The changes to make_sky_grid relate to some design considerations I made in #4995.

I am not a big fan of structuring code like this:

if some_condition:
    # do something short and simple
else:
    # do something very long and complicated

I would prefer something like this instead:

def make_single_det_grid(args):
    # do something short and simple

def make_multi_det_grid(args):
    # do something very long and complicated

if one_det_given:
    make_single_det_grid(args)
else:
    make_multi_det_grid()

There is also the consideration that @Thomas-JACQUOT is working on a deep rewrite of make_skymap anyway to support the HEALPix input… Maybe we should spend some time at the next call to find a good design for the various use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PyGRB PyGRB development
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants