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

remove wrong shim thickness and typo for top back UChannel #1001

Merged
merged 3 commits into from
Jul 24, 2023

Conversation

sarahgaiser
Copy link
Collaborator

Removes 700um shift that Cam notices during his Survey Constant analysis and fixes bug in geometry code. The bug was as follows: by not setting the node for the top back alignment corrections, the corresponding survey volume had no access to the survey constants defined in the compact.xml. Fixing the typo solves this problem.

Copy link
Collaborator

@cbravo135 cbravo135 left a comment

Choose a reason for hiding this comment

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

I checked out this code and made comparisons between the 2016 and 2019 detectors and confirmed this does indeed fix the issue of getting different detectors with the same survey data.

@cbravo135 cbravo135 requested a review from pbutti July 19, 2023 23:23
@@ -556,7 +556,7 @@ public static class ModuleL3Bot extends ModuleL13Bot {
//2019 MRSolt survey
//protected final static double L3_new_vertical_shift = 0.7 - 0.012;
//2021 nominal
protected final static double L3_new_vertical_shift = 0.7;
protected final static double L3_new_vertical_shift = 0.;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any clue why Matt Added this 0.7mm. ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My best guess is this was to account for the shims added wrt the engineering run detector. I don't think it was that thick, will confirm with Tim. Also, this isn't even used if you supply survey data for the modules.

@@ -51,7 +51,7 @@ public void build() {
AlignmentCorrection supBotCorrBack = getUChannelCorrection(false,90);
supBotCorrBack.setNode(node);
AlignmentCorrection supTopCorrBack = getUChannelCorrection(true,90);
supTopCorr.setNode(node);
supTopCorrBack.setNode(node);
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed a bug!

Copy link
Collaborator

@cbravo135 cbravo135 left a comment

Choose a reason for hiding this comment

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

Need to properly update detector lcdd files.

@sarahgaiser sarahgaiser force-pushed the sarahgaiser-patch-1 branch from eaee4b2 to a00570d Compare July 20, 2023 18:11
@normangraf
Copy link
Contributor

Please hold off on merging this into master until we can discuss this at the software meeting. I would much prefer that we maintain backwards-compatibility with four years of simulation and reconstruction work. Changing the geometry in this way will invalidate those detectors. We should modify the compact.xml files to be commensurate with the new code such that the output geometry is unchanged. We should then proceed from here on out with new detectors that correct the mistake.

Copy link
Collaborator

@cbravo135 cbravo135 left a comment

Choose a reason for hiding this comment

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

We have not done much simulation/reconstruction work in the past 4 years. If we need to rereco some MC for some reason we can always use the 5.1 release to have the same detectors. Making things "backwards compatible" to be able to keep a bug around is poor software practice.

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