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.

Sign up for GitHub

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

Jump to bottom

Add ACTS tracking in all CIs #1518

Open

tvami wants to merge 3 commits into trunk from iss1517-tracking-prop-in-ecal

+701 −25

Member

tvami commented Jan 18, 2025 •

edited

Loading

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

What are the issues that this addresses?

First step to resolve #1517

Resolves #1487

Check List

I successfully compiled ldmx-sw with my developments
I ran my developments and the following shows that they are successful.

tvami added 2 commits

January 17, 2025 18:39


          Add ACTS tracking in all CIs

8cb3f39


          Dont switch the tracking in ecal on yet

226e1f5

tvami requested review from tomeichlersmith, bryngemark and fdelzanno

January 18, 2025 02:47


          Move to ldmx-reduced-v2 in CI

c2ecde5

tomeichlersmith requested changes

View reviewed changes

Member

tomeichlersmith left a comment

My only request is focused on using the newer syntax for configuring the logger. While both work, since these configs are used as reference by many folks, I'd like them to contain the newer syntax as example.

.github/validation_samples/ecal_pn/config.py

    
              # The Tracking modules produce a lot of helpful messages

              # but (at the debug level) is too much for commiting the gold log

              # into the git working tree on GitHub

              p.termLogLevel = 0

Member

tomeichlersmith Jan 21, 2025

Do you want to use the newer syntax? (I also want to double check that #1458 works.)

Suggested change

      
            p.termLogLevel = 0
          
            p.logger.termLevel = 0

.github/validation_samples/inclusive/config.py

+              # The Tracking modules produce a lot of helpful messages
+              # but (at the debug level) is too much for commiting the gold log
+              # into the git working tree on GitHub
               p.termLogLevel = 0

Member

tomeichlersmith Jan 21, 2025

Suggested change

      
            p.termLogLevel = 0
          
            p.logger.termLevel = 0

.github/validation_samples/inclusive/config.py

+              # The Tracking modules produce a lot of helpful messages
+              # but (at the debug level) is too much for commiting the gold log
+              # into the git working tree on GitHub
+              p.termLogLevel = 0

Member

tomeichlersmith Jan 21, 2025

Suggested change

      
            p.termLogLevel = 0
          
            p.logger.termLevel = 0

.github/validation_samples/it_pileup/config.py

    
              # The Tracking modules produce a lot of helpful messages

              # but (at the debug level) is too much for commiting the gold log

              # into the git working tree on GitHub

              p.termLogLevel = 0

Member

tomeichlersmith Jan 21, 2025

Suggested change

      
            p.termLogLevel = 0
          
            p.logger.termLevel = 0

.github/validation_samples/kaon_enhanced/config.py

    
              # The Tracking modules produce a lot of helpful messages

              # but (at the debug level) is too much for commiting the gold log

              # into the git working tree on GitHub

              p.termLogLevel = 0

Member

tomeichlersmith Jan 21, 2025

Suggested change

      
            p.termLogLevel = 0
          
            p.logger.termLevel = 0

.github/validation_samples/signal/config.py

    
              # The Tracking modules produce a lot of helpful messages

              # but (at the debug level) is too much for commiting the gold log

              # into the git working tree on GitHub

              p.termLogLevel = 0

Member

tomeichlersmith Jan 21, 2025

Suggested change

      
            p.termLogLevel = 0
          
            p.logger.termLevel = 0

fdelzanno approved these changes

View reviewed changes

fdelzanno left a comment

I assume the values of the cuts on the SeedFinding (i.e. pmin, pmax, d0min, d0max, etc.) are up to date from the tracking people (I haven't followed their developments in the last weeks)? If so, the implementation looks good to me.

Contributor

bryngemark commented Jan 21, 2025

I assume the values of the cuts on the SeedFinding (i.e. pmin, pmax, d0min, d0max, etc.) are up to date from the tracking people

I agree with this comment, but even more so, I think whatever has been settled on in recent tracking optimizations should go in as defaults for all these processors, and not be specified in detail here. As @tomeichlersmith pointed out, the validation configs are very useful examples and having people set all kinds of variables themselves is not sustainable in the long term.

Member Author

tvami commented Jan 21, 2025

@bloodyyugo @EBerzin can you please confirm?

Contributor

bloodyyugo commented Jan 21, 2025

I agree with this comment, but even more so, I think whatever has been settled on in recent tracking optimizations should go in as defaults for all these processors, and not be specified in detail here. As @tomeichlersmith pointed out, the validation configs are very useful examples and having people set all kinds of variables themselves is not sustainable in the long term.

Yeah, these values are good. Setting them as default values is a good idea, but we use the same class for both tagger and recoil tracking and they potentially have different values (though right now they are so loose they might as well be the same). That said, we could easily add a switch to tell the code if it's doing tagger or recoil and set the parameters appropriately.

I plan on changing how these cuts are implemented in the nearish future, so I can add this then.

Contributor

EBerzin commented Jan 22, 2025

The values look good for the tagger. But these config files use RecoilTruthSeeds for recoil tracking, which are much cleaner than reconstructed seeds. In this case the parameters that are set for seeder_recoil I think actually don't matter. But if we want to use reconstructed seeds in these configs, the optimized parameters I found are a bit different, and the full tracking chain also requires an ambiguity resolution step. I was planning to open a PR with those ambiguity resolution changes later today, which will also include an example config with the optimized seeding parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet