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

Add filter to remove nan and silly candidates #374

Merged
merged 4 commits into from
Oct 27, 2021

Conversation

mmasciov
Copy link
Collaborator

@mmasciov mmasciov commented Oct 25, 2021

PR description

This PR introduces a filter on 'silly' candidates, to avoid error/warning messages in CMSSW, addressing #373.

PR validation

Physics performance is unchanged.
Timing performance is also ~ unchanged (in the plots linked below there is an offset in the measurement; however, after subtracting such offset, timing is indeed unchanged).
MTV results based on TTbar (=50): http://uaf-10.t2.ucsd.edu/~mmasciov/MkFit_devs/MTV_testNanNSillyRemoval/
Running on 100 events, the size of the log file decreases from 3.6 MB to 1.3 MB
(note that the warning suppression introduced by Slava in cms-sw/cmssw#35802 is not included).
The instances of 'candidate ignored' decrease from 4778 to 53.

@osschar
Copy link
Collaborator

osschar commented Oct 25, 2021

Filter-style removal makes sense between forward-search and backward fit. It's somewhat expensive as it has to move comb-cands up in the vector once one of them gets removed. We do run all filters at the same time, don't we?

I thought we need to to do this at the very end ... so why not just have the check in export function ... and simply not export the CombCand for which this fails.

How about seeds? Do we want to check there, too? There was a "fix" option (that didn't make much sense to me and I remember asking what should actually be done with those seeds / or what beter values to put there would be / or where this would need to be fixed upstream (in the seed producer)).

@mmasciov
Copy link
Collaborator Author

mmasciov commented Oct 25, 2021

Filter-style removal makes sense between forward-search and backward fit. It's somewhat expensive as it has to move comb-cands up in the vector once one of them gets removed. We do run all filters at the same time, don't we?

I thought we need to to do this at the very end ... so why not just have the check in export function ... and simply not export the CombCand for which this fails.

How about seeds? Do we want to check there, too? There was a "fix" option (that didn't make much sense to me and I remember asking what should actually be done with those seeds / or what beter values to put there would be / or where this would need to be fixed upstream (in the seed producer)).

This is done at the very end, after backward fit:
https://github.com/trackreco/mkFit/pull/374/files#diff-9bd117b4474e1261389e0f33d9f898dc985c780ab0e7c9c12339ce68867a0e66R715
There are also new filters applied at the end.
It could probably be done also during the export, I guess.

As for seeds, currently we assign an error of 1e-5.
We could try to remove the bad seeds instead (there is an option, disabled by default, which can remove silly seeds in the seed post-processing, instead if 'fixing' them.

@osschar
Copy link
Collaborator

osschar commented Oct 25, 2021

OK, then let's leave the final filter optimization for next time (maybe passing a vector of filter functions to export) :)

I don't know what to do with bad seeds ... and how many of them there even are in each iteration.

@mmasciov
Copy link
Collaborator Author

OK, then let's leave the final filter optimization for next time (maybe passing a vector of filter functions to export) :)

I don't know what to do with bad seeds ... and how many of them there even are in each iteration.

I have enabled by default the removal of silly seeds with commit 9ef01f6, if any. No difference, as this is very unlikely to happen:
http://uaf-10.t2.ucsd.edu/~mmasciov/MkFit_devs/MTV_testNanNSillyRemoval_sillySeedRemoval/

Track.cc Outdated
Comment on lines 186 to 187
if ( ! is_silly )
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is the if ( ! is_silly ) needed here?
should it just return here instead of continuing to loop over i

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it's right. Pushing an update.

@slava77 slava77 merged commit 9ec7037 into devel Oct 27, 2021
@osschar osschar deleted the removeNanNSillyCandidates branch December 14, 2021 23:41
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.

3 participants