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

Complete the migration from PhysicsTools/CandUtils #22584

Merged
merged 16 commits into from Mar 13, 2018

Conversation

perrotta
Copy link
Contributor

As pointed out in the issue #22483 , nearly all classes "migrated" by 60bfee7 have still a copy in PhysicsTools/CandUtils

Code in CMSSW accessed either one or the other implementation, with the same name and often also with the definition in the interface protected by an identical include guard

This PR removes all the duplicate implementations that were left in PhysicsTools/CandUtils, and adapts the classes that still relied on them to use the same (identical) implementations that were migrated longtime ago in CommonTools/CandUtils

@steggema @makortel @rekovic @roger-wolf @slava77

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@rekovic
Copy link
Contributor

rekovic commented Mar 12, 2018

@perrotta Thanks very much for this PR.
Would it be possible to have this PR back-ported to 9_3_X?
We should probably do it for 10_0_X as well.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@perrotta
Copy link
Contributor Author

perrotta commented Mar 12, 2018 via email

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 8 differences found in the comparisons
  • DQMHistoTests: Total files compared: 29
  • DQMHistoTests: Total histograms compared: 2477919
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2477742
  • DQMHistoTests: Total skipped: 176
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.619999999784 KiB( 23 files compared)
  • Checked 118 log files, 9 edm output root files, 29 DQM output files

@rekovic
Copy link
Contributor

rekovic commented Mar 12, 2018

Ok for 93X.

Great. Thanks.

I don't think it will be needed in 10_0, however: what usage you have
in mind for it?

Was not sure if anybody was planning to do any MC production or major Re-RECO where this would
come up. I have to admit that the only place that I am aware of where this was causing seg faults
were Phase-II workflows. Maybe there are others around the corner, so out of precaution thought it
would be a good idea.

@rekovic
Copy link
Contributor

rekovic commented Mar 13, 2018

Jenkins comparisons show no difference. Good!

@perrotta, @cmsbuild, @silviodonato, @fwyzard, @monttj, @Martin-Grunewald, @slava77, @gpetruc, @arizzi can you please review it and eventually sign?

Need a backport of it asap in 93x.

@perrotta
Copy link
Contributor Author

+1

  • Duplicated classes in PhysicsTools/CandUtils are been removed, and all code in CMSSW that still accessed them gets modified accordingly
  • No effect is expected in reco outputs, none is observed in jenkins tests

@perrotta
Copy link
Contributor Author

@rekovic : backport for 93X in #22588

@Martin-Grunewald
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 4d5e4d2 into cms-sw:master Mar 13, 2018
@rekovic
Copy link
Contributor

rekovic commented Mar 13, 2018

@perrotta, @Martin-Grunewald @fabiocos
Thanks.

backport for 93X in #22588

OK. Thanks.

@perrotta perrotta deleted the completeMigrationFromPhysicsTools branch March 13, 2018 16:59
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

5 participants