eic / EICrecon

EIC Reconstruction - JANA based
https://eic.github.io/EICrecon
GNU Lesser General Public License v3.0
6 stars 29 forks source link

fixed LFHCAL adjacency matrix #1488

Closed steinber closed 4 months ago

steinber commented 4 months ago

Briefly, what does this PR introduce?

Fix to LFHCAL adjacency matrix, which was splitting essentially all clusters before, due to a sign flip in the local towerx,y convention

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

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

No changes needed - rather people should see better LFHCAL clustering behavior

Does this PR change default behavior?

Yes, since it avoids splitting clusters in the LFHCAL

wdconinc commented 4 months ago

@veprbl for backport v1.14 (label added)

veprbl commented 4 months ago

@steinber We can't run our full CI for PRs submitted from a fork. Would you mind pushing your lfhcal-adjacency-fix branch to the git@github.com:eic/EICrecon.git and submitting a PR from that. Thanks.

steinber commented 4 months ago

@veprbl I believe I have done this.

wdconinc commented 4 months ago

@veprbl I believe I have done this.

As long as the top of the PR says steinber:lfhcal-adjacency-fix, with something before the colon, it is not a branch in this repository. Likely you will need to do, from inside your local copy and with your branch checked out:

git remote add eic git@github.com:eic/EICrecon.git
git push eic HEAD
steinber commented 4 months ago

@veprbl I believe I have done this.

As long as the top of the PR says steinber:lfhcal-adjacency-fix, with something before the colon, it is not a branch in this repository. Likely you will need to do, from inside your local copy and with your branch checked out:

git remote add eic git@github.com:eic/EICrecon.git
git push eic HEAD

I pushed my branch directly to eic/EICrecon.git and opened a new PR

https://github.com/eic/EICrecon/pull/1493

This seems to get the job done, but I'm happy to hear otherwise.

wdconinc commented 4 months ago

Closing in favor of #1493.