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

Migrate EcalDeadChannelRecoveryBDTG to GBRForest #28040

Closed
wants to merge 1 commit into from
Closed

Migrate EcalDeadChannelRecoveryBDTG to GBRForest #28040

wants to merge 1 commit into from

Conversation

guitargeek
Copy link
Contributor

PR description:

I don't know what the plans are to resolve #27648, but as I already wrote the code for the migration to find out if it would be possible and compared the output with TMVA (see #27175 (comment)), I thought I might as well do a PR with the code from back then.

PR validation:

CMSSW compiles, local matrix tests pass. However, I'm not sure if the comparisons would catch possible problems. Is the channel recovery now enabled by default so the matrix tests would capture possible differences? That would make validating this PR easier.

if this PR is a backport please specify the original PR:

No backport intended.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28040/11972

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @guitargeek (Jonas Rembser) for master.

It involves the following packages:

RecoLocalCalo/EcalDeadChannelRecoveryAlgos
RecoLocalCalo/EcalRecProducers

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@apsallid, @argiro 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

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 20, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2602/console Started: 2019/09/21 00:07

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

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

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 12 differences found in the comparisons
  • DQMHistoTests: Total files compared: 34
  • DQMHistoTests: Total histograms compared: 2958092
  • DQMHistoTests: Total failures: 129
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2957622
  • DQMHistoTests: Total skipped: 341
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
  • Checked 145 log files, 15 edm output root files, 34 DQM output files

@perrotta
Copy link
Contributor

@nancymarinelli @taroni FYI

@nancymarinelli
Copy link
Contributor

nancymarinelli commented Sep 21, 2019 via email

@perrotta
Copy link
Contributor

Changes observed in the number of conversion photons in Phase2 wf 11634.0 are spurious ones.
The same was observed in PR #28048 and #28020.

As I did never notice it before the last couple of days, something should have been merged since then which oririginates such a non-reproducibility in this Phase2 workflow (in all three cases it is the same wf)

@fabiocos @slava77 @kpedro88

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 23, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2626/console Started: 2019/09/23 22:19

@guitargeek guitargeek closed this Jul 25, 2020
@guitargeek guitargeek reopened this Dec 14, 2020
@guitargeek
Copy link
Contributor Author

Just reopening this PR to check in. After all there is still a corresponding issue #27648 open.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28040/20398

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

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

@perrotta
Copy link
Contributor

please test
(just to keep it hot)

@perrotta
Copy link
Contributor

@thomreis since this PR has been momentarily re-opened for technical reasons, let me profit to inquire with you about the status of the validation of the EcalDeadChannelRecovery: are there updates since last July (bi-annual check, see #28040 (comment))?

@nancymarinelli
Copy link
Contributor

nancymarinelli commented Dec 15, 2020 via email

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-55d22b/11658/summary.html
CMSSW: CMSSW_11_3_X_2020-12-14-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: 36
  • DQMHistoTests: Total histograms compared: 2747284
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2747261
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
  • Checked 153 log files, 37 edm output root files, 36 DQM output files

@thomreis
Copy link
Contributor

@perrotta to my knowledge there are no updates to the validation status. At least I do not find any related presentation in ECAL meetings of the last half year.

@nancymarinelli
Copy link
Contributor

nancymarinelli commented Dec 23, 2020 via email

@perrotta
Copy link
Contributor

Thank you @thomreis and @nancymarinelli
@guitargeek , I think you can put to bed this PR once again...

@guitargeek guitargeek closed this Dec 24, 2020
@fcouderc
Copy link
Contributor

fcouderc commented Jan 4, 2021

@perrotta , @nancymarinelli

Meanwhile we have had some discussion with the JME group about the validation and they did not see any issue popping out (though the first results were not encouraging, it was due to a bug I believe). I am adding JME conveners (@lathomas @ahinzmann ) to confirm that the validation is ok on their side.

Sorry for the late notice.

Cheers

Fabrice & Simone

@lathomas
Copy link
Contributor

lathomas commented Jan 4, 2021

(adding @kirschen )
Hello,
I can confirm we didn't see any issue, but also no improvement, including for electrons, which sounds a bit surprising. Could someone point us to an ECAL/EGM study showing the expected effect?
Thanks !

@nancymarinelli
Copy link
Contributor

nancymarinelli commented Jan 7, 2021 via email

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.

migrate EcalDeadChannelRecoveryBDTG to GBRForest