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: replace RawPMTHit with RawTrackerHit #508

Merged
merged 6 commits into from Mar 8, 2023

Conversation

c-dilks
Copy link
Member

@c-dilks c-dilks commented Feb 24, 2023

Briefly, what does this PR introduce?

edm4eic::RawPMTHit is basically a duplicate datatype of edm4eic::RawTrackerHit, so let's use RawTrackerHit and deprecate RawPMTHit.

See also eic/EDM4eic#22

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • Other: datatype change

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • 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 (except in #393)

Does this PR change default behavior?

No

@c-dilks c-dilks changed the title chore: replace RawPMTHit with RawTrackerHit refactor: replace RawPMTHit with RawTrackerHit Feb 27, 2023
@c-dilks c-dilks marked this pull request as ready for review February 28, 2023 01:02
@c-dilks
Copy link
Member Author

c-dilks commented Feb 28, 2023

CI failures are unrelated to this PR (and currently happening on main).

@wdconinc
Copy link
Contributor

Why is it segfaulting now? Just noticed in main too ...

wdconinc
wdconinc previously approved these changes Mar 3, 2023
Copy link
Contributor

@wdconinc wdconinc 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. Minor suggestion for future resiliency.

src/algorithms/digi/PhotoMultiplierHitDigi.cc Outdated Show resolved Hide resolved
@wdconinc wdconinc enabled auto-merge March 3, 2023 03:33
@wdconinc
Copy link
Contributor

wdconinc commented Mar 3, 2023

Set to auto-merge, pending unresolved conversation.

@c-dilks c-dilks requested a review from wdconinc March 3, 2023 17:02
@c-dilks
Copy link
Member Author

c-dilks commented Mar 3, 2023

I also applied these changes to the Juggler MR

@c-dilks
Copy link
Member Author

c-dilks commented Mar 3, 2023

Same exact CI failures are happening on unrelated PR #524, so it seems like this is a general problem.

@c-dilks
Copy link
Member Author

c-dilks commented Mar 3, 2023

Oddly enough, the last push to main ran fine...

@wdconinc
Copy link
Contributor

wdconinc commented Mar 3, 2023

Can you restart a job on main? Might be due to jana upgrade...

@c-dilks
Copy link
Member Author

c-dilks commented Mar 3, 2023

Sure, re-running the last PR merge commit: https://github.com/eic/EICrecon/actions/runs/4320254537 (which passed all CI checks before this)

@c-dilks
Copy link
Member Author

c-dilks commented Mar 3, 2023

Same failures

@wdconinc
Copy link
Contributor

wdconinc commented Mar 3, 2023

And it's one of those 'fun' failures that doesn't fail inside valgrind...

@wdconinc wdconinc merged commit f537c76 into main Mar 8, 2023
35 checks passed
@wdconinc wdconinc deleted the RawPMTHit_to_RawTrackerHit branch March 8, 2023 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants