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

Fixes to the tauID configuration tool #37456

Merged
merged 1 commit into from
Apr 13, 2022

Conversation

mbluj
Copy link
Contributor

@mbluj mbluj commented Apr 4, 2022

PR description:

This is a technical PR which fixes two small issues in tauID configuration tools:

  • Modification of RecoTauTag/RecoTau/python/tools/runTauIdMVA.py is to correctly deal with cases when the tool is used two times in the same "process"; The change does not affect workflows in which the tool is used (mini and nanoAOD)
  • Modification of adaptToRunAtMiniAOD.py adds BDT-based tauIDs to boostedTau workflow on top of miniAOD. These tauIDs were removed in Removal of depreciated tauIDs #37091 from configuration from which this workflow is derived. The boostedTau-at-mini workflow is not run as a part of official workflows thus the change does not affect them.

PR validation:

Validated in workflows using the modified tools - miniAOD and nanoAOD production, and tau-at-miniAOD workflows.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37456/29151

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 4, 2022

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

It involves the following packages:

  • RecoTauTag/Configuration (reconstruction)
  • RecoTauTag/RecoTau (reconstruction)

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

cms-bot commands are listed here

@jpata
Copy link
Contributor

jpata commented Apr 7, 2022

@cmsbuild please test

@jpata
Copy link
Contributor

jpata commented Apr 7, 2022

assign xpog

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2022

New categories assigned: xpog

@mariadalfonso,@gouskos,@fgolf you have been requested to review this Pull request/Issue and eventually sign? Thanks

@jpata
Copy link
Contributor

jpata commented Apr 7, 2022

Something might be off with the whitespace here, could you please check (e.g. tabs vs. spaces)?
Screenshot from 2022-04-07 15-45-17

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37456/29195

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 7, 2022

Pull request #37456 was updated. @gouskos, @clacaputo, @cmsbuild, @fgolf, @slava77, @jpata, @mariadalfonso can you please check and sign again.

@mbluj
Copy link
Contributor Author

mbluj commented Apr 7, 2022

Something might be off with the whitespace here, could you please check (e.g. tabs vs. spaces)? Screenshot from 2022-04-07 15-45-17

I tried to fix it and it looks fine in my editor. Then I force pushed ("push -f"), but in web gui it looks as before. I have not idea how to align it better, do you have any suggestion?

@jpata
Copy link
Contributor

jpata commented Apr 7, 2022

You can confirm it by making your editor show the actual tab character, rather than some arbitrary amount of spaces instead of the tab. I use vim, where ":set list" gives the following
Screenshot from 2022-04-07 18-39-35

confirming that the file uses tabs, while you used spaces.
EDIT: You may need to tell your editor to not convert tabs to spaces automatically while working on this file. There should also be no need to force push (to be avoided in general, unless rebasing).

@mbluj
Copy link
Contributor Author

mbluj commented Apr 8, 2022

@jpata I replaced spaces by tabs as used in other places (emacs which I use is fussy with it), and I hope that it solves the issue with alignment.
EDIT: sorry for forced pushes - I agree that it is better to avoid them, but in this case it is used to not inflate history with simple cosmetic changes.

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2022

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-37456/29211

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2022

Pull request #37456 was updated. @gouskos, @clacaputo, @cmsbuild, @fgolf, @slava77, @jpata, @mariadalfonso can you please check and sign again.

@mariadalfonso
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Apr 8, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-61d500/23779/summary.html
COMMIT: ae19ad8
CMSSW: CMSSW_12_4_X_2022-04-08-1100/slc7_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/37456/23779/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: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3593039
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3593015
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 200 log files, 45 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@jpata
Copy link
Contributor

jpata commented Apr 12, 2022

+reconstruction

  • config fixes for non-production code

@mariadalfonso
Copy link
Contributor

+xpog

no change in nanos as expected

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

@qliphy
Copy link
Contributor

qliphy commented Apr 13, 2022

+1

@cmsbuild cmsbuild merged commit 8e5e96a into cms-sw:master Apr 13, 2022
@mbluj mbluj deleted the CMSSW_12_4_X_tauToolFixes branch March 13, 2024 13:25
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.

5 participants