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

Iss1181 - Hcal position reconstruction + geometry verifier #1182

Merged
merged 7 commits into from
Aug 4, 2023

Conversation

EinarElen
Copy link
Contributor

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

This resolves #1181 and patches one part of #1166 (missing case statement in getScintillatorOrientation.

This updates the hcal geometry condition so that the map of strip centers aligns with the GDML. I will say that there almost certainly are more elegant expressions than what I ended up using, so if anyone has a suggestion then feel more than free to suggest them.

One thing which caused me quite a lot of headache before I caught it was that the entire Hcal geometry is offset by 19.05 mm in y. I'm not entirely sure why this is, but I've added a configuration parameter that handles this.

Since this was quite fiddly, I wanted to make sure we really got this right so I also added an analyzer in the DQM module which goes through all simulated and reconstructed hits in the hcal and checks if the position is within the bounding box of the corresponding scintillator bar. For the v14 geometry this is 100% correct, excluding reco issues in #1348.

I have not gone through a similar exercise for v12.

Check List

  • I successfully compiled ldmx-sw with my developments

  • I ran my developments and the following shows that they are successful.
    See analyzer above.

  • I attached any sub-module related changes to this PR.
    N/A

Related Sub-Module PRs

N/A

@EinarElen
Copy link
Contributor Author

I would expect this to fail some of the validation figures, anything that has to do with Hcal positions are going to change.

@tomeichlersmith
Copy link
Member

the entire Hcal geometry is offset by 19.05 mm in y

@cmantill is correct - I implemented this offset because the ECal support box is slightly asymmetric in the vertical direction (it has to have a base to rest on but it is open at the top for the layers to be slid in like a filing cabinet). I had to slide the ECal volume up so that the sensitive layers were still centered on the beam and thus I had to do the same for the HCal. I apologize for wasting your time from my lack of documenting this.

I would expect this to fail some of the validation figures, anything that has to do with Hcal positions are going to change.

That makes sense - do you want to include your geometry verifier in the validation histograms? Maybe in the hcal validation sample? If we need to recreate the golden histograms anyways, now is a good time to introduce new ones.

@EinarElen
Copy link
Contributor Author

That would explain it 😅, I've obviously been staring at the stuff for too long because I completely missed that when looking for offsets in the GDML...

Regarding the verifier, I'm not 100% convinced that it is going to be super useful at the moment but yeah maybe on the hcal one. Ideally, if we didn't have #1348 I think it would be quite nice to have it run with quitting on mismatch, that would catch position reco bugs early but before that has been patched that's not an option.

@EinarElen
Copy link
Contributor Author

All the failing tests are HcalDQM tests related to the side Hcal :)

Copy link
Member

@tomeichlersmith tomeichlersmith 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 and code is understandable. I'm approving this since I think it is ready to be merged in. If you want to add the geo verifier to the CI now on this branch, I'm happy to re-review. But we can also wait to pull that verifier into the CI until the HCal reco bug you mentioned is resolved.

@EinarElen
Copy link
Contributor Author

I also think we can merge it. We can return to the CI question later if we want.

@tomeichlersmith tomeichlersmith merged commit c2773df into trunk Aug 4, 2023
@tomeichlersmith tomeichlersmith deleted the iss1181 branch August 4, 2023 12:53
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.

Side Hcal position reconstruction
3 participants