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

Make MuonTagger thread safe #8304

Merged
merged 1 commit into from Mar 17, 2015

Conversation

Dr15Jones
Copy link
Contributor

Since the MuonTagger can be obtained via the EventSetup, the
const methods of the class must be thread safe. This required replacing
TRandom3 with C++11 random facility (since TRandom3 uses a global seed)
and using an std::mutex to protect MvaSoftMuonEstimator since the
way it interacts with TMVA::Reader is thread-unsafe.

Since the MuonTagger can be obtained via the EventSetup, the
const methods of the class must be thread safe. This required replacing
TRandom3 with C++11 random facility (since TRandom3 uses a global seed)
and using an std::mutex to protect MvaSoftMuonEstimator since the
way it interacts with TMVA::Reader is thread-unsafe.
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for CMSSW_7_4_X.

Make MuonTagger thread safe

It involves the following packages:

RecoBTag/SoftLepton

@cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please review it and eventually sign? Thanks.
@ferencek, @acaudron, @pvmulder, @imarches this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@@ -35,8 +36,8 @@ float MuonTagger::discriminator(const TagInfoHelper& tagInfo) const {
bool flip(false);
if(m_selector.isNegative()) {
int seed=1+round(10000.*properties.deltaR);
Copy link
Contributor

Choose a reason for hiding this comment

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

a random (that is not so random) in reco ???
Could somebody explain the rationale of all this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is all random numbers used in RECO are seeded deterministically. The reason is to guarantee we get the same results each time we run the RECO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I confirm.
The RECO algorithms should in general not contain any randomness.
The best is if the random generator is not used at all, but in cases where it's used the algorithm seed is event/object-specific so that the results are reproducible.

@cmsbuild
Copy link
Contributor

@slava77
Copy link
Contributor

slava77 commented Mar 16, 2015

+1

for #8304 6346525

in addition to jenkins, tested locally with higher stats in CMSSW_7_4_X_2015-03-14-1400 /test area sign529/

there are no changes in monitored quantities, which suggests that the code affected by the random number selection/generation is actually not triggered (requires multiple candidates).

I ran igprof on ttbar with pu35@25ns and observed some minor increase in CPU in the two affected modules (by this and #8284 PR)

        softPFMuonBJetTags       0.65958 ms/ev -> 0.853296 ms/ev
        softPFElectronBJetTags   2.2377 ms/ev -> 2.42587 ms/ev

the increase is, unconvincingly, matching the igprof cost in MvaSoftEleEstimator::mvaValue, but maybe a jitter. The random engine-related elements are not showing up in igprof.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @ktf, @smuzaffar

@davidlange6
Copy link
Contributor

+1

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

Successfully merging this pull request may close these issues.

None yet

5 participants