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

Update for changes in allegro ecal segmentation. #135

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

scott-snyder
Copy link
Contributor

@scott-snyder scott-snyder commented Jan 22, 2025

The allegro ecal segmentation class has been changed to properly return a position in the volume-local coordinate system. Remove the workaround here that was dealing with what the segmentation used to return.

BEGINRELEASENOTES

  • Update cell position calculation for Allegro ECal to go along with changes in k4geo.
    ENDRELEASENOTES

The allegro ecal segmentation class has been changed to properly return
a position in the volume-local coordinate system.  Remove the workaround
here that was dealing with what the segmentation used to return.
@scott-snyder
Copy link
Contributor Author

Depends on PR#1384 from DD4hep (AIDASoft/DD4hep#1384) and PR#423 from k4geo (key4hep/k4geo#423).

@scott-snyder
Copy link
Contributor Author

Now that the k4geo change was merged, this should also get merged. Otherwise, cell positions will be wrong.

@scott-snyder
Copy link
Contributor Author

The failures are because we're not seeing the updated version of k4geo, on which this depends.
I'll try to put in a temporary hack again to work around.

@giovannimarchiori
Copy link
Contributor

Hi @scott-snyder
I don't think we need a work around. Tomorrow the nightly will have the k4geo with your changes merged in and we can rerun the tests and they should succeed - then we merge

Copy link
Contributor

@giovannimarchiori giovannimarchiori left a comment

Choose a reason for hiding this comment

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

The changes look good to me, they are basically straightforward given the way the underlying code in k4geo (and DD4hep) has been reshuffled.
Maybe one comment is that the release notes are a bit misleading, the reader could think that the cells were badly positioned before, which is not the case - the user will get the same positioning with this positioning tool before/after changes.

@scott-snyder
Copy link
Contributor Author

Thanks. I tried to update the description slightly to be clearer.
I'll hold off on making any changes for now. However, i would expect the tests against the release would still fail, since the k4geo (and dd4hep) changes wouldn't have gotten there. (That was the last holdup in getting the k4geo change merged.)

@giovannimarchiori
Copy link
Contributor

Mmm ok, for k4RecCalorimeter it usually suffices that the tests are passed for the nightlies for us to accept the pr..

@jmcarcell
Copy link
Contributor

I just ran the CI and the nightlies pass @giovannimarchiori. The release fails as expected.

@giovannimarchiori
Copy link
Contributor

Tests for nightlies passed successfully, going to merge

@giovannimarchiori giovannimarchiori merged commit e0cef74 into HEP-FCC:main Feb 5, 2025
2 of 4 checks passed
@giovannimarchiori
Copy link
Contributor

just to confirm that with 1000 photon events (10 GeV, theta between 50 and 130 degrees) the cell positions (with the position tool) are identical before/after merging this PR, and that the hits from the readout (summing the G4 steps with same cellID) have now the proper z position

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

3 participants