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

Fix GenHFHadronMatcher [master] #25026

Merged
merged 3 commits into from Nov 9, 2018

Conversation

Dr15Jones
Copy link
Contributor

The nanoAOD jobs were crashing in GenHFHadronMatcher because of a missing simulation parentage of a tau. This was fixed with the first commit.
Took the opportunity to modernize the module, especially since it is being used in an 8 thread job.

Avoid a segmentation fault in the case of a tau particle not
having a mother particle. This was seen during nanoAOD processing.
Although the ParticleDataTable was pulled from the EventSetup,
it was never used.
It was possible to change all member data of GenHFHadronMatcher to
be const and the module was not calling any thread unsafe external
functions. It was therefore easy to convert to a global module.
In addition, it now used EDPutTokens and edm::Event::emplace which
further improve the efficiency of the module.
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 26, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31305/console Started: 2018/10/26 18:47

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

PhysicsTools/JetMCAlgos

@cmsbuild, @fgolf, @peruzzim, @monttj can you please review it and eventually sign? Thanks.
@imarches, @smoortga, @acaudron, @HeinerTholen, @JyothsnaKomaragiri, @mverzett, @ferencek, @pvmulder this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@zhenhu
Copy link
Contributor

zhenhu commented Oct 26, 2018

Hi @Dr15Jones , you may also want to backport it to 10_2_X ?

@Dr15Jones
Copy link
Contributor Author

Hi @Dr15Jones , you may also want to backport it to 10_2_X ?

I will leave that to others if there is a need.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@zhenhu
Copy link
Contributor

zhenhu commented Oct 26, 2018

please test workflow 10887.0,11087.0

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 26, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31316/console Started: 2018/10/26 23:54

@fabiocos
Copy link
Contributor

fabiocos commented Nov 1, 2018

please test workflow 10887.0,11087.0

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 1, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/31413/console Started: 2018/11/01 10:47

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 1, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 1, 2018

Comparison job queued.

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 1, 2018

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 32
  • DQMHistoTests: Total histograms compared: 2993155
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2992956
  • DQMHistoTests: Total skipped: 197
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 31 files compared)
  • Checked 134 log files, 14 edm output root files, 32 DQM output files

@zhenhu
Copy link
Contributor

zhenhu commented Nov 1, 2018

please test workflow 10887.0,11087.0

Hi @fabiocos , for some reason, 10887.0,11087.0 didn't run, and I could not find the logs for them.

@fabiocos
Copy link
Contributor

fabiocos commented Nov 2, 2018

@zhenhu the wfs 10887.0,11087.0 are not part of the standard test library, they are in the upgrade section of runTheMatrix that can be accessed through the option --what upgrade. This is not accessible to my knowledge in the bot tests (@smuzaffar do you confirm ?)

Running these tests in a normal test area I see them successful with the PR, please cross-check

@fabiocos
Copy link
Contributor

fabiocos commented Nov 2, 2018

@ahinzmann @zdemirag please notice this fix, looking at the code I assume it is ok, but it is unclear to me how to test it. I see a test cfg that seem not functional because of input files not found, and anyway one should have a quick way of inspecting the tree content. Do you have this kind of tests?

@ahinzmann
Copy link
Contributor

@mverzett @kskovpen @ferencek It would be good if you could have a look at this hadron definition? (since its main consumer is b-tagging obviously)
Adding also the JME RECO contacts @rappoccio @dnash86 @gkasieczka.

@fabiocos
Copy link
Contributor

fabiocos commented Nov 8, 2018

@ahinzmann any news on this?

@ahinzmann
Copy link
Contributor

@mverzett @kskovpen @ferencek @rappoccio @dnash86 @gkasieczka Can you please comment?

@mverzett
Copy link
Contributor

mverzett commented Nov 9, 2018

The real changes are minimal look good to me and thanks for updating the code to a more modern style.

@fabiocos
Copy link
Contributor

fabiocos commented Nov 9, 2018

+1

@fabiocos
Copy link
Contributor

fabiocos commented Nov 9, 2018

merge

@cmsbuild cmsbuild merged commit e0293b2 into cms-sw:master Nov 9, 2018
@ferencek
Copy link
Contributor

Apologies for the late response. I haven't noticed right away I was mentioned here.

@ahinzmann this flavour code was introduced back in 2014 for some specific analyses in the Higgs PAG, if I remember correctly. It is actually not used by the BTV POG and I am not really sure who would claim the ownership of this code right now.

In any case, thanks to @Dr15Jones for the fix and other code improvements.

@Dr15Jones Dr15Jones deleted the fixGenHFHadronMatcher_master branch November 12, 2018 17:27
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

7 participants