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

Use make_unique in tau modules #42447

Merged
merged 3 commits into from
Aug 18, 2023
Merged

Conversation

mbluj
Copy link
Contributor

@mbluj mbluj commented Aug 2, 2023

PR description:

Technical PR which consistently introduces usage of std::unique_ptr in tau modules, namely:

  • bare C pointers replaced by std::unique_ptr in places overlooked in previous developments,
  • std::unique_ptr consequently initialized with std::make_unique rather than with new operator.

In addition, DeepTauId::produce method is now explicitly ended when the processed tau collection is empty which prevents of handling other collections, building TF objects, etc. and hopefully will avoid throwing exceptions in events without taus (#40437, #42444).

Those changes are not expected to modify outputs.

Will it be useful to backport this PR to 13_0/1/2?

PR validation:

Successfully tested with a custom tau@miniAOD workflow (10k events) and with runTheMatrix.py -l limited -i all --ibeos

@cmsbuild cmsbuild added this to the CMSSW_13_3_X milestone Aug 2, 2023
@mbluj mbluj changed the title Cmssw 13 2 x makeunique Use make_unique in tau modules Aug 2, 2023
@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 2, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42447/36456

  • This PR adds an extra 88KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 2, 2023

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

It involves the following packages:

  • RecoTauTag/RecoTau (reconstruction)

@cmsbuild, @mandrenguyen, @clacaputo can you please review it and eventually sign? Thanks.
@mbluj, @missirol, @azotz this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

…ection is empty, and other small improvements
@mbluj
Copy link
Contributor Author

mbluj commented Aug 3, 2023

Review comments partly addressed while waiting on core's suggestions about changes concerning the "by pointer vs by value" issue.

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 3, 2023

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-42447/36463

  • This PR adds an extra 84KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 3, 2023

Pull request #42447 was updated. @cmsbuild, @mandrenguyen, @clacaputo can you please check and sign again.

@mbluj
Copy link
Contributor Author

mbluj commented Aug 9, 2023

Review comments partly addressed while waiting on core's suggestions about changes concerning the "by pointer vs by value" issue.

It was agreed to prepare a followup PR to address the "by pointer vs by value" which requires extensive changes in tau modules. Could this PR be tested and then merged?
Will it be interesting to to backport it to older releases for production (which ones)?

(Note: I will be offline since next week until end of August, so it will be good to close it in this week).

@mandrenguyen
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3feabf/34286/summary.html
COMMIT: dc112a6
CMSSW: CMSSW_13_3_X_2023-08-14-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/42447/34286/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 10 lines from the logs
  • Reco comparison results: 14 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3152843
  • DQMHistoTests: Total failures: 9
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3152812
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@mandrenguyen
Copy link
Contributor

+1

@cmsbuild
Copy link
Contributor

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

@rappoccio
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 24e0ef4 into cms-sw:master Aug 18, 2023
11 checks passed
@mbluj mbluj deleted the CMSSW_13_2_X_makeunique branch March 13, 2024 13:26
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

6 participants