fix index typo in PFRecoTauClusterVariables

[slava77]

@swozniewski @mbluj

I expect that this should get rid of non-reproducible behavior in phase-2 workflows in tau ID outputs

[cmsbuild]

The code-checks are being triggered in jenkins.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30919/17314

• This PR adds an extra 16KB to repository

[cmsbuild]

A new Pull Request was created by @slava77 (Slava Krutelyov) for master.

It involves the following packages:

RecoTauTag/RecoTau

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@riga this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

[slava77]

@cmsbuild please test

[cmsbuild]

The tests are being triggered in jenkins.

• CMSSW_11_2_X_2020-07-25-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/8292/console Started: 2020/07/25 23:25

[cmsbuild]

+1
Tested at: fdb86b8
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8e562d/8292/summary.html
CMSSW: CMSSW_11_2_X_2020-07-25-1100
SCRAM_ARCH: slc7_amd64_gcc820

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

• [Clang tidy] Apply checks for fastsim #30796 @cms-sw: [Clang tidy] Apply checks for fastsim
• Properly set number of threads in CmsRun python module #30908 @Dr15Jones: Properly set number of threads in CmsRun python module

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8e562d/8292/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8e562d/8292/git-merge-result

[cmsbuild]

Comparison job queued.

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8e562d/8292/summary.html

Comparison Summary:

• No significant changes to the logs found
• Reco comparison results: 13 differences found in the comparisons
• DQMHistoTests: Total files compared: 34
• DQMHistoTests: Total histograms compared: 2526188
• DQMHistoTests: Total failures: 7
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2526134
• DQMHistoTests: Total skipped: 47
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
• Checked 144 log files, 17 edm output root files, 34 DQM output files

[slava77]

@swozniewski @mbluj @fojensen
please clarify where is the original code that does the mapping between the names/indices of variables in the training payload.

I'd naively expect this to still be available in the BDT file metadata.
I looked at the RecoTauTag_tauIdMVAIsoPhase2 (SHA f8e6ade86304851fcd1a3194c299fea8a2453294) and it doesn't have the metadata for variable names.

[swozniewski]

@slava77 thank you for the fast follow-up! I agree that this is probably the right solution but I cannot tell for sure. @fojensen could you please verify with your training setup?

[slava77]

+1

for #30919 fdb86b8

• this should at least solve the reproducibility problems; confirmation of the complete correct variable mapping is pending

[cmsbuild]

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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

[silviodonato]

+1

[fojensen]

Apolgies for wasting your time with my typo in the original PR. Thank you very much for noticing it though. The code which does the training is in a local area of mine, I just checked and I agree with the choice of index you have assigned. Regarding the metadata, is this something which would have been introduced when adding the variables at the training stage, i.e. the TMVA AddVariable function? https://root.cern.ch/doc/v612/classTMVA_1_1DataLoader.html#af2de13debc441fd2c4f1cd826ef175f9 Apologies, I did not fill in that information when I ran the training.

…

On Jul 26, 2020, at 10:06 AM, Slava Krutelyov ***@***.***> wrote: @swozniewski @mbluj @fojensen please clarify where is the original code that does the mapping between the names/indices of variables in the training payload. I'd naively expect this to still be available in the BDT file metadata. I looked at the RecoTauTag_tauIdMVAIsoPhase2 (SHA f8e6ade86304851fcd1a3194c299fea8a2453294) and it doesn't have the metadata for variable names. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[slava77]

Regarding the metadata, is this something which would have been introduced when adding the variables at the training stage, i.e. the TMVA AddVariable function?

apparently, not. At least not in the way I was thinking when I asked the question.
The payloads are of GBRForest and they do not contain any data about variable names.
It would be useful to keep a reference link to the code that originally extracted the variables in the description of #30341 (or perhaps even inlined in the code that's making MVA inputs).
This will allow for at least more open way to cross-reference the meaning of the variables.

This in a way goes back to #29818 .