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

Ported DDSimpleMuonDigi #1

Closed
wants to merge 0 commits into from

Conversation

kkostova16
Copy link

Ported DDSimpleMuonDigi reconstruction algorithm from DDMarlinPandora to Gaudi.

Copy link
Contributor

@jmcarcell jmcarcell left a comment

Choose a reason for hiding this comment

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

In several places comments related to the old code can be removed, that's my general comment for now.

k4GaudiPandora/CMakeLists.txt Outdated Show resolved Hide resolved
k4GaudiPandora/include/DDSimpleMuonDigi.h Outdated Show resolved Hide resolved
k4GaudiPandora/include/DDSimpleMuonDigi.h Outdated Show resolved Hide resolved
k4GaudiPandora/src/DDSimpleMuonDigi.cc Outdated Show resolved Hide resolved
k4GaudiPandora/src/DDSimpleMuonDigi.cc Outdated Show resolved Hide resolved
k4GaudiPandora/src/DDSimpleMuonDigi.cc Outdated Show resolved Hide resolved
k4GaudiPandora/src/DDSimpleMuonDigi.cc Outdated Show resolved Hide resolved
Copy link

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

I would like to advocate for some modernization of parts of this since it is worked on in any case at the moment.

k4GaudiPandora/include/CalorimeterHitType.h Outdated Show resolved Hide resolved
k4GaudiPandora/include/CalorimeterHitType.h Outdated Show resolved Hide resolved
k4GaudiPandora/include/CalorimeterHitType.h Outdated Show resolved Hide resolved
@andresailer
Copy link
Contributor

Let's not lose the attribution to @SwathiSasikumar please.

@andresailer
Copy link
Contributor

@jmcarcell
Copy link
Contributor

Let's not lose the attribution to @SwathiSasikumar please.

rebasedPortedAlgorithms (commits)

But here there are a lot of commits we don't need, it's all the history of DDMarlinPandora and it would refer to non-existing files in the repo. And also there are many commits about formatting here that should be squashed together. We can change authorship in a squashed commit of this PR, or Swathi and Katerina can put the @author tag inside the algorithm.

@andresailer
Copy link
Contributor

Why shouldn't we keep the history of DDMarlinPandora?

@jmcarcell
Copy link
Contributor

Why should we? It's a disjoint set of files that is never accessible from here and one can always go back to DDMarlinPandora. If you are wondering about a change in a file here that would never be in the history from DDMarlinPandora. And what happens if now someone changes something in DDMarlinPandora? Two versions of the history of the processor? Isn't just easy to separate the history of the processor and the algorithm?

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@jmcarcell jmcarcell force-pushed the ported-algorithms branch 4 times, most recently from 175bd79 to ef9266f Compare August 6, 2024 14:37
@andresailer
Copy link
Contributor

And what happens if now someone changes something in DDMarlinPandora?

Then we can see that we don't have that change in our history here, so that is an argument for keeping the history?

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.

4 participants