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

Further preparations of deep tau ID for hlt phase2 #32676

Merged

Conversation

swozniewski
Copy link
Contributor

PR description:

Modifications of the DeepTauID code for phase II tau trigger studies:

  • add auxiliary functionality like a check of input IDs, file dump of input quantities.
  • add options disable_CellIndex_workaround, disable_hcalFraction_workaround to turn off workarounds which won't be needed in the future.
  • add option enableHGCalWorkaround to PFRecoTauDiscriminationByIsolation serving as input to DeepTauID to enable a workaround needed for phase II.
  • Changed default value for variables related to number of hits to 0 instead of -999
  • Change the puppiweight to have an additional argument to define its return value for AOD. This allows the puppiweights to be defined separately for different blocks. Additionally split between weights for inner and outer blocks. All values have been extracted from the training.

Clean up of defaults in DeepTauID functions introduced during last preparation of phase II trigger studies (PR #31744 ):

  • Remove the default_value constant from all deepTau v2 code
  • Removed default_value argument from certain candfunctions and instead use conditionals where needed in the places these functions are called.

Modifications/defaults preserve the current DeepTauID for PAT level and no physics changes are intended there.
DeepTauID on AOD as introduced recently in PR #31744 is developing and changes are intended.

PR validation:

  • Basic check by the developers: Reran config file with update from deepTauAtHLT.py and everything ran smoothly. Running RecoTauTag/RecoTau/test/runDeepTauIDsOnMiniAOD.py also ran fine. Upon brief inspection, no strange behaviour observed.
  • code checks and format applied
  • unit tests pass
  • limited matrix tests pass

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32676/20773

  • This PR adds an extra 60KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @swozniewski for master.

It involves the following packages:

RecoTauTag/RecoTau

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@@ -415,6 +423,22 @@ namespace {
}
} // namespace dnn_inputs_2017_v2

float getTauID(const pat::Tau& tau, const std::string& tauID, float default_value = -999.) {
std::lock_guard<std::mutex> guard(g_mutex);
Copy link
Contributor

Choose a reason for hiding this comment

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

  • guard can be placed inside else section to not block execution for the cases when tau id is available.
  • mutex can be defined as static inside getTauID instead of using the global namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

A better solution would be to use something like tbb::concurrent_unordered_set<std::string> for isFirstWarning, and check with something like

if (isFirstWarning.insert(tauID).second) {
   edm::LogWarning("TauID") << ...
}

which doesn't require any explicit locks.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32676/20775

  • This PR adds an extra 60KB to repository

@cmsbuild
Copy link
Contributor

Pull request #32676 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Jan 18, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0b3e31/12320/summary.html
COMMIT: 1ab315e
CMSSW: CMSSW_11_3_X_2021-01-17-2300/slc7_amd64_gcc900

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2716961
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2716938
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 156 log files, 37 edm output root files, 37 DQM output files

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32676/20787

  • This PR adds an extra 64KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@swozniewski
Copy link
Contributor Author

apparently I was a bit naive about what small changes require different formatting. To be fixed shortly...

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 5, 2021

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32676/21025

  • This PR adds an extra 180KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 5, 2021

Pull request #32676 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Feb 5, 2021

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 5, 2021

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0b3e31/12734/summary.html
COMMIT: 036963d
CMSSW: CMSSW_11_3_X_2021-02-05-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/32676/12734/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 7 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2751765
  • DQMHistoTests: Total failures: 12
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2751730
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 156 log files, 37 edm output root files, 37 DQM output files

@swozniewski
Copy link
Contributor Author

The quantities where the seven differences show up are not known to me and do not seem to be tau related. Please let me know if you think that these differences are in fact due to the latest commit.

@slava77
Copy link
Contributor

slava77 commented Feb 5, 2021

+1

for #32676 036963d

  • code changes are in line with the PR description and the follow up review; the changes are technical in the context of the use cases in miniAOD
  • jenkins tests pass and comparisons with the baseline show no relevant differences
  • I managed to run the "online" variant as described in Further preparations of deep tau ID for hlt phase2 #32676 (comment) somewhat blindly in CMSSW_11_3_X_2021-02-04-1100 (after a small fix in config of PFBlockProducer, a change since the 11_2_0 version of GRun)

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 5, 2021

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)

@qliphy
Copy link
Contributor

qliphy commented Feb 5, 2021

+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

8 participants