Skip to content

Feature/tm sconverer#9

Merged
chenel merged 26 commits intomasterfrom
feature/TMSconverer
Aug 25, 2022
Merged

Feature/tm sconverer#9
chenel merged 26 commits intomasterfrom
feature/TMSconverer

Conversation

@cmmarshall
Copy link
Collaborator

TMS reco/matching merge

@cmmarshall cmmarshall requested review from chenel and cjbacchus August 17, 2022 10:07
Copy link
Contributor

@cjbacchus cjbacchus left a comment

Choose a reason for hiding this comment

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

Looks basically sane to me. We decided to sequence this after the similar SAND merge since they will likely slightly conflict.

Copy link
Collaborator

@chenel chenel left a comment

Choose a reason for hiding this comment

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

generally ok---a few suggestions and questions but once Chris's comments are addressed I think we're ready to merge

@cjbacchus
Copy link
Contributor

I pushed the fixes that make this work with the newly-merged SAND branch

@cjbacchus
Copy link
Contributor

Is someone working on the most major of the comments above?

@clarenc3
Copy link
Contributor

@cjbackhouse I plan to address them either today or over the weekend. I agree with all the above comments for what it's worth.

Faiza is updating the track matcher so I'll wait for her and let you know

…into the TMS instead of propagating both tracks into the gap between LAr and TMS; this choice was made because trusting the direction vector of a bending track in the TMS isnt the best idea). Also update method of not relying on TF1, instead just doing the arithmetic in the code. Also move to doubles instead of floats. And allow for multiple TMS tracks to match a single LAr track and vice versa; dont stop looking for matches after one is found
@clarenc3
Copy link
Contributor

Branch is now updated with a few additions:

  1. Remove TF1 reliance and instead just use arithmetic with doubles
  2. Update matching code to propagate the LAr track into the first layer of the TMS and do the residual on that point, instead of propagating both tracks into the dead region between the TMS and LAr. This removes reliance on the TMS unit vector for calculating the residual; the TMS unit vector can sometimes be unreliable because a straight line is a bad parameterisation of the track in the TMS due to the bending
  3. Make TMS use cm as default unit for distances; LAr already does this and TMS was using mm. Also update the track matching code to know about this.

Found some bugs during this; the LAr direction vectors appear to be corrupted. Hence the matching calculates the line in LAr by using start and end points of the LAr track rather than the saved direction vector. This should be updated when the LAr unit vectors are fixed.

@chenel
Copy link
Collaborator

chenel commented Aug 24, 2022

I think we are in a position to merge this (the ND-LAr bug above is not related to anything in this repo and will be fixed next time we respin). @clarenc3 would you do the honors?

@chenel
Copy link
Collaborator

chenel commented Aug 25, 2022

Turns out @clarenc3 is ✈️ today, so I hereby invoke the merge. 🪄

@chenel chenel merged commit 38fdc6f into master Aug 25, 2022
@chenel chenel deleted the feature/TMSconverer branch August 25, 2022 12:42
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.

5 participants