search

cms-L1TK / cmssw

Fork of CMSSW where improvements to L1 tracking code are developed.
http://cms-sw.github.io/
Apache License 2.0
4 stars 5 forks source link

Find best MC match #174

Closed bryates closed 2 years ago

bryates commented 2 years ago

PR description: This PR adds a better implementation of the ability to find better FullMatch memories. The code is similar to the HLS implementation, where for a particular tracklet, the best values are stored, and new stubs are only matched if their delta values (z, r, phi depending on barrel or disk matches) are better than the current best.

PR validation: We were already seeing full agreement with 100 events in the barrel. However, while developing and testing the HLS MC disk implementation, I've seen this gives more correct matches (the HLS implementation is stull under development so not all matches agree yet).

tomalin commented 2 years ago

This looks fine. As you clearly understand this code, could you please put a comment at the start of MatchCalculator.h explaining what this class does. And add comments whereever the code looks obscure, and you've figured out what it does.

P.S. Is a similar fix needed to the MatchProcessor?

bryates commented 2 years ago

This looks fine. As you clearly understand this code, could you please put a comment at the start of MatchCalculator.h explaining what this class does. And add comments whereever the code looks obscure, and you've figured out what it does.

P.S. Is a similar fix needed to the MatchProcessor?

Yes, we will need this in the MP as well. Would you like me to include it in this PR?

tomalin commented 2 years ago

This looks fine. As you clearly understand this code, could you please put a comment at the start of MatchCalculator.h explaining what this class does. And add comments whereever the code looks obscure, and you've figured out what it does. P.S. Is a similar fix needed to the MatchProcessor?

Yes, we will need this in the MP as well. Would you like me to include it in this PR?

I don't mind. Either fix MP with separate PR or include it in this one.

bryates commented 2 years ago

This looks fine. As you clearly understand this code, could you please put a comment at the start of MatchCalculator.h explaining what this class does. And add comments whereever the code looks obscure, and you've figured out what it does. P.S. Is a similar fix needed to the MatchProcessor?

Yes, we will need this in the MP as well. Would you like me to include it in this PR?

I don't mind. Either fix MP with separate PR or include it in this one.

I was looking a bit more into the MP and I don't think we will need it. The MP is implemented very differently. I'll be porting the HLS disk implementation soon, so that should be a good test to make sure.

tomalin commented 2 years ago

I can approve this once the comments are added to the code.

bryates commented 2 years ago

I can approve this once the comments are added to the code.

Sorry I missed your initial request for more comments. I've added a header, and some comments to parts of the code where it wasn't as straight forward as the rest.