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

Patching segmentlists in pycbc.workflow module #5008

Merged
merged 2 commits into from
Jan 20, 2025

Conversation

Thomas-JACQUOT
Copy link
Contributor

Standard information about the request

This is a bug fix (in order to fix issue #5004)

This change affects PyGRB's workflows

This change was proposed by @duncanmmacleod in #5004 in this comment.

Links to any issues or associated PRs

This is the linked issue : #5004

Testing performed

This change was tested by submitting a PyGRB workflow that succeed to start.

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

@spxiwh
Copy link
Contributor

spxiwh commented Jan 17, 2025

I would prefer the following:

if isinstance(segs, segments.segment):
    self.segment_list = segments.segmentlist([segs])
elif isinstance(segs, segments.segmentlist):
    self.segment_list = segs
else:
    if not isinstance(segs, list):
        segs = [segs]
    try:
        self.segment_list = segments.segmentlist(map(segments.segment, segs))
    except ValueError as exc:
        exc.args = (
            "segs input must be either igwn_segments.segment or "
            f"igwn_segments.segmentlist. Got {type(segs).__name__}.",
        )
        raise

which keeps the initial isinstance checks. The reason is that if we have a long list (which can be the case) the map thing will (I think) duplicate a bunch of stuff, and is potentially slow. Not copying the segmentlist object when we don't need to would be more efficient.

@sebastiangomezlopez sebastiangomezlopez added PyGRB PyGRB development bug labels Jan 17, 2025
Copy link
Contributor

@spxiwh spxiwh 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!

@titodalcanton titodalcanton merged commit 52034ec into gwastro:master Jan 20, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug PyGRB PyGRB development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants