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

refactor+fix: use collections and multi-factory in RichTrack_factory; fix potential leaks in TrackPropagation #656

Merged
merged 4 commits into from
May 29, 2023

Conversation

c-dilks
Copy link
Member

@c-dilks c-dilks commented May 7, 2023

Briefly, what does this PR introduce?

  • return Collection of TrackSegments in TrackPropagation::propagateToSurfaceList, rather than a (potentially leaky) new TrackSegment pointer
    • the loop over input trajectories is moved from RichTrack_factory to TrackPropagation::propagateToSurfaceList, which makes the PR diff appear to be more complex than it actually is
    • fix additional possible memory leaks by replacing any return new raw_pointer calls with return std::make_unique<...>(...), and update any code which depends on this change
  • improve the configuration with RichTrackConfig, which configures the factory, not the algorithm, since the factory is used to connect to the richgeo service (and not the more general TrackPropagation algorithm)

What kind of change does this PR introduce?

  • Bug fix (memory leaks)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added: tested in irt-algo branch
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

no, unless the TrackPropagation algorithm is called by anything external of EICrecon

Does this PR change default behavior?

no

@c-dilks c-dilks self-assigned this May 7, 2023
@c-dilks c-dilks mentioned this pull request May 7, 2023
30 tasks
@c-dilks c-dilks changed the title feat(dRICH): use collections and multi-factory in RichTrack_factory refactor(dRICH): use collections and multi-factory in RichTrack_factory May 7, 2023
@c-dilks c-dilks force-pushed the rich-track-multifactory branch from 852c733 to 29fe147 Compare May 9, 2023 14:02
@c-dilks c-dilks requested review from DraTeots, wdconinc and sly2j May 9, 2023 15:37
@c-dilks c-dilks changed the title refactor(dRICH): use collections and multi-factory in RichTrack_factory refactor+fix: use collections and multi-factory in RichTrack_factory; fix potential leaks in TrackPropagation May 9, 2023
@c-dilks c-dilks force-pushed the rich-track-multifactory branch from 1c9e1a8 to b325164 Compare May 17, 2023 00:08
@c-dilks c-dilks force-pushed the rich-track-multifactory branch from b325164 to 399c497 Compare May 24, 2023 03:23
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

It looks like there was a memory leak to fix here. Nice find.

src/algorithms/tracking/TrackPropagation.cc Show resolved Hide resolved
src/algorithms/tracking/TrackPropagation.cc Show resolved Hide resolved
@c-dilks
Copy link
Member Author

c-dilks commented May 24, 2023

It looks like there was a memory leak to fix here. Nice find.

Possibly more at #661.

@c-dilks c-dilks force-pushed the rich-track-multifactory branch from 399c497 to 1c63180 Compare May 26, 2023 20:33
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

No additional comments

Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

No additional comments

@c-dilks c-dilks merged commit c51579a into main May 29, 2023
@c-dilks c-dilks deleted the rich-track-multifactory branch May 29, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants