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

Add Pass2 2021 detector with a coarse alignment #901

Merged
merged 2 commits into from
Mar 16, 2022
Merged

Add Pass2 2021 detector with a coarse alignment #901

merged 2 commits into from
Mar 16, 2022

Conversation

cbravo135
Copy link
Collaborator

This PR just adds a new detector after some work has been done to improve alignment. The biggest improvement with this detector is a much closer momentum scale for electron tracks in the bottom volume.

@bloodyyugo bloodyyugo self-assigned this Mar 7, 2022
@bloodyyugo bloodyyugo self-requested a review March 7, 2022 21:37
@bloodyyugo bloodyyugo removed their assignment Mar 7, 2022
@cbravo135
Copy link
Collaborator Author

Ok, the detector is correct in the branch now.

@normangraf
Copy link
Contributor

Please provide some additional information on how this detector was aligned, e.g. which detector was used as a starting point, which alignment parameters were allowed to float, what constraints were used, etc.
Is this detector now mature or should it be left on a branch for further development?

@cbravo135
Copy link
Collaborator Author

cbravo135 commented Mar 15, 2022

Here is a link to the slides for the talk I gave today on this detector:

https://confluence.slac.stanford.edu/pages/viewpage.action?pageId=341248576&preview=/341248576/341248917/220315_feePass2ali.pdf

@cbravo135
Copy link
Collaborator Author

It is unclear to me what you mean by "mature". I would say it is more "mature" than HPS_Run2021Pass1Top, and also every other 2021 geometry currently available. Looking at the number of detectors available for each year in master, it seems the precedent is to put snapshots of the detector into master as progress is made.

@cbravo135
Copy link
Collaborator Author

I don't ever plan to update this particular detector, and future improvements will be submitted with a new detector name.

@cbravo135 cbravo135 requested a review from pbutti March 15, 2022 20:38
@normangraf
Copy link
Contributor

The talk you referenced does not mention this detector by name, so it's not clear what went into this alignment. The presentation also contains a number of qualifiers, which is why I asked whether this detector will be developed further.

Still some improvement to be made
The beam spot doesn’t seem to be in the correct position currently
Unclear if top and bottom even agree on beamspot position
Thinking about studying moving beamspot to more positions
Top and bottom don’t quite agree on where the vertex is in z
Still have some major issues we need to figure out

@normangraf
Copy link
Contributor

Why is this detector being merged into master? This is a 2021 detector and until the Run2021 branch gets merged into master this geometry will conflict with the hard-coded geometry parameters in the 2019 code base.

@cbravo135
Copy link
Collaborator Author

I can point out several other detectors in master than have similar or worse issues. There is clearly no precedent for only putting fully 100% good detectors into master.

1 similar comment
@cbravo135
Copy link
Collaborator Author

I can point out several other detectors in master than have similar or worse issues. There is clearly no precedent for only putting fully 100% good detectors into master.

@cbravo135 cbravo135 merged commit 0f337b3 into master Mar 16, 2022
@cbravo135
Copy link
Collaborator Author

My comment above in this PR makes it clear this detector is the one the talk is about. There are still some improvements to be made to the 2016 geometry, but that didn't stop anyone from putting in the many versions of the 2016 detector into master.

This pull request was closed.
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