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
Fix bug in candidate seeded mode of AreaSeededTrackingRegionsBuilder #22310
Fix bug in candidate seeded mode of AreaSeededTrackingRegionsBuilder #22310
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22310/3514 |
A new Pull Request was created by @JanFSchulte (Jan-Frederik Schulte) for master. It involves the following packages: RecoTracker/TkTrackingRegions @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
If it is only about using
from reco::Track s. Maybe it would be easier to use than copy-pasting code around?
|
@makortel Oh, that looks useful indeed and I will use it to simplify the PR |
There is also |
The code-checks are being triggered in jenkins. |
1 similar comment
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22310/3517 |
@makortel 99% of the PR were indeed not needed. What remains is the bug that the full pixel volume is used if the number of candidates is zero in candidate seeded mode, which still needs to be fix. |
@JanFSchulte If it is not too much of a trouble, could you squash your 3 commits to 1? I think in this case the commit history simplification would be well justified. |
@makortel Is there fancy git magic for this, or do I just create a new PR with a single commit? (which is no problem) |
See e.g. http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html and then force-push the same branch. |
c6ee725
to
8aa9975
Compare
The code-checks are being triggered in jenkins. |
Neat. Thanks for the instructions. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-22310/3518 |
@cmsbuild, please test Thanks Jan. |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
@JanFSchulte : (just to know it) what is it supposed to happen when the number of candidates is zero and the method returns a nullptr? |
@perrotta It is caught here https://github.com/JanFSchulte/cmssw/blob/8aa9975b39cc4dd63d67be3d2d7de26a08b8ff58/RecoTracker/TkTrackingRegions/plugins/AreaSeededTrackingRegionsBuilder.cc#L70 and no region is actually added to the result of the producer. |
+1
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
This PR fixes a bug in the candidate seeded option. I noticed that when the number of candidates is zero, the current implementation falls back to running the recovery in the full pixel volume, which is of course not desired. This PR fixes this with a proper check if the candidate seeded option is on.