JetCorrector migration in DQM/Physics #39847

wddgit · 2022-10-25T17:01:33Z

PR description:

This is part of the migration from getting the deprecated ::JetCorrector class from the EventSetup to getting the new reco::JetCorrector from the Event. This migration has been in progress since 2014 and is nearly complete. There are only a few cases left that need migrating. We want to finish this soon and delete the deprecated header file and related code. This is related to the consumes migration and will improve multi-threading performance.

This PR handles the remaining cases in the package DQM/Physics. I need some help with this from DQM if possible.

First question. Is this code still used? If not, I would rather delete it entirely than continue maintaining dead code.

The existing code has what appears to be a bug. See for example, https://cmssdt.cern.ch/dxr/CMSSW/source/DQM/Physics/src/SingleTopTChannelLeptonDQM.cc#511

The following conditional always evaluates to false.

if (!jetCorrector_.isInitialized() && jetCorrector_.hasValidIndex())

So the code inside that conditional that uses the deprecated JetCorrector class is never actually executed. From the history, it appears this is not intentional. If it was intentional, I could just delete the related code fragments, not modify the behavior, and complete the migration. If you want this, please let me know.

In the PR, I have migrated the C++ code to use the new interface in reco::JetCorrector. I need help, because I do not know how to test this code. I also do not know how to verify that the configurations are correct and reading the correct JetCorrector objects. Is there a DQM expert who could help with this? Which tests should I run? I am willing to do a little more work on this, but need some help and/or direction.

There are no unit tests. I've run the limited runTheMatrix.py tests, but I do not know if they use this code or not. If I know what tests to run, then I will try to run them. There may be implications for changing histograms and needing to update comparison reference histograms and DQM experts would need to handle that.

I am a Core expert, not a DQM expert so help from DQM on this migration would be appreciated.

Note the old JetCorrectors for the EventSetup are no longer supported. I'm not sure that they would run without exceptions even if the existing bug was fixed.

PR validation:

It compiles and builds. There are no unit tests. Limited runTheMatrix tests pass, but I do not know if those execute this code.

wddgit · 2022-10-25T17:03:18Z

FYI @makortel @Dr15Jones

cmsbuild · 2022-10-25T17:12:24Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39847/32726

This PR adds an extra 40KB to repository

wddgit · 2022-10-25T17:12:37Z

please test

cmsbuild · 2022-10-25T17:12:45Z

A new Pull Request was created by @wddgit (W. David Dagenhart) for master.

It involves the following packages:

DQM/Physics (dqm)

@emanueleusai, @ahmad3213, @jfernan2, @syuvivida, @pmandrik, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks.
@rappoccio 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

cmsbuild · 2022-10-25T20:20:21Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7fb13a/28500/summary.html
COMMIT: 65cd4fd
CMSSW: CMSSW_12_6_X_2022-10-25-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39847/28500/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: 7 differences found in the comparisons
Reco comparison had 6 failed jobs
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3378384
DQMHistoTests: Total failures: 9
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3378353
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 201 log files, 48 edm output root files, 48 DQM output files
TriggerResults: no differences found

emanueleusai · 2022-10-28T05:50:15Z

+1

cmsbuild · 2022-10-28T05:50:35Z

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

perrotta · 2022-10-28T07:29:51Z

@emanueleusai rather that simply signing this PR, could you please review and answer the several questions to @cms-sw/dqm-l2 that were posed by @wddgit in the PR description?
It looks like that either this code must be fixed (by someone in the dqm group, I guess) or you should confirm that this code is not in use, and therefore it can be removed thus saving people to spend time in maintenance.
Please let us know. For the moment, I put this PR in hold, waiting for your feedback.

perrotta · 2022-10-28T07:30:15Z

hold
(waiting for some input from @cms-sw/dqm-l2 )

cmsbuild · 2022-10-28T07:30:36Z

Pull request has been put on hold by @perrotta
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

wddgit · 2022-11-07T16:13:03Z

New info related to this: The PR that deletes the deprecated header file (JetCorrector.h) was submitted, #39953. I tested it on top of this PR (and 3 other similar ones), all PR tests pass. Further, I ran runTheMatrix.py (the full version with all 1000+ tests) and that passes. It successfully compiles and builds.

The only high priority question that I see that someone should answer immediately is whether the pre-existing bug causes some critical problem that must be fixed right away. Since it is DQM and not reconstruction, my guess is that the answer is no. It seems like at worst we would be creating tuples or histograms without jet corrections to be used only for monitoring.

We can create an issue, remember it, and when an expert is available revisit this and investigate carefully. This and possibly other DQM code that is unused could be deleted later, when it is convenient.

I am willing to modify the PR and approach it in a different way. Let me know if you want this. Two options I can think of:

Delete the code.
Replace the code using the deprecated JetCorrector with code that always throws.

emanueleusai · 2022-11-08T01:48:53Z

@wddgit No, to the best of my knowledge, it does not break anything essential. We are running a survey of all POG/PAG-related DQM modules and check which one are actually still being looked at. In particular we strongly suspect the top-related modules object of this PR are run in our Physics DQM sequence, but are not actually used by anybody. Please allow us a few more days to verify with the interested parties (TOP group) that these modules are not used so we can delete them altogether.
In any case, running these modules with uncorrected jets would not be a critical problem given, as you say, it's used only for monitoring.

wddgit · 2022-11-14T15:03:57Z

Any news on this yet? A week ago there were 4 PRs left under review that remove dependence on the old JetCorrector class. Two were merged. Only this one and one other are left now. We are getting close.

emanueleusai · 2022-11-15T03:21:05Z

@wddgit we are checking if the sequences are used for RelVal, we should have an answer soon.

wddgit · 2022-11-21T16:20:40Z

Any news yet? I am eagerly waiting on this PR to be able to delete the old JetCorrector code (PR #39953).

wddgit · 2022-11-28T15:50:05Z

Any news yet?

emanueleusai · 2022-11-29T04:58:23Z

The module is actually run in the Physics sequence in DQM Offline and RelVal, so we cannot remove it or make it throw an exception every time. Being part of the relval DQM and not seeing any differences in the DQM bin-to-bin I believe the module runs OK.
Concerning the bug, I believe you are right and I would leave it fixed as you did in the PR.

The overarching question, which should not hold this PR, is whether the module is still needed and somebody is actually looking at the plots in Physics/TOP. This is something we are investigating and will take a little longer to sort out. So we can delete these modules in a later PR if it turns out they are not needed anymore.

My vote goes for merging as it is and perhaps open an issue to investigate whether the module is still needed.

wddgit · 2022-11-29T14:00:18Z

@emanueleusai That sounds good to me.

wddgit · 2022-12-01T19:20:46Z

@perrotta I think we arrived at the point where we are waiting for you to remove the hold (since you placed that hold). @emanueleusai commented 3 days ago and those comments resolved the remaining issues, unless there is something I am missing.

Maybe it is worth refreshing the tests. I will start them now.

wddgit · 2022-12-01T19:20:56Z

please test

perrotta · 2022-12-01T20:43:14Z

unhold

cmsbuild · 2022-12-01T20:43:40Z

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

cmsbuild · 2022-12-01T23:11:34Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7fb13a/29405/summary.html
COMMIT: 65cd4fd
CMSSW: CMSSW_13_0_X_2022-12-01-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39847/29405/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: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 48
DQMHistoTests: Total histograms compared: 3421159
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3421137
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
Checked 206 log files, 158 edm output root files, 48 DQM output files
TriggerResults: no differences found

perrotta · 2022-12-02T05:29:49Z

+1

JetCorrector migration in DQM/Physics

65cd4fd

cmsbuild added this to the CMSSW_12_6_X milestone Oct 25, 2022

cmsbuild added code-checks-pending dqm-pending orp-pending pending-signatures tests-pending labels Oct 25, 2022

cmsbuild added code-checks-approved tests-started and removed tests-pending code-checks-pending labels Oct 25, 2022

cmsbuild added tests-approved and removed tests-started labels Oct 25, 2022

cmsbuild added dqm-approved fully-signed and removed dqm-pending pending-signatures labels Oct 28, 2022

cmsbuild added hold pending-signatures and removed fully-signed labels Oct 28, 2022

cmsbuild modified the milestones: CMSSW_12_6_X, CMSSW_13_0_X Nov 24, 2022

cmsbuild mentioned this pull request Nov 30, 2022

[DQM] [LLVM14] Apply code checks #40186

Merged

cmsbuild added tests-started and removed tests-approved labels Dec 1, 2022

cmsbuild added fully-signed and removed pending-signatures hold labels Dec 1, 2022

cmsbuild added tests-approved and removed tests-started labels Dec 1, 2022

cmsbuild added orp-approved and removed orp-pending labels Dec 2, 2022

cmsbuild merged commit 2d9ae37 into cms-sw:master Dec 2, 2022

This was referenced Dec 2, 2022

[G4VECGEOM] Update clhep to 2.4.6.0, vecgeom to 1.2.1 cms-sw/cmsdist#8211

Merged

Update ROOT's llvm to llvm13 cms-sw/root#177

Closed

wddgit deleted the dqmJetCorrectorMigration branch January 13, 2023 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JetCorrector migration in DQM/Physics #39847

JetCorrector migration in DQM/Physics #39847

wddgit commented Oct 25, 2022

wddgit commented Oct 25, 2022

cmsbuild commented Oct 25, 2022

wddgit commented Oct 25, 2022

cmsbuild commented Oct 25, 2022

cmsbuild commented Oct 25, 2022

emanueleusai commented Oct 28, 2022

cmsbuild commented Oct 28, 2022

perrotta commented Oct 28, 2022

perrotta commented Oct 28, 2022

cmsbuild commented Oct 28, 2022

wddgit commented Nov 7, 2022

emanueleusai commented Nov 8, 2022

wddgit commented Nov 14, 2022

emanueleusai commented Nov 15, 2022

wddgit commented Nov 21, 2022

wddgit commented Nov 28, 2022

emanueleusai commented Nov 29, 2022

wddgit commented Nov 29, 2022

wddgit commented Dec 1, 2022

wddgit commented Dec 1, 2022

perrotta commented Dec 1, 2022

cmsbuild commented Dec 1, 2022

cmsbuild commented Dec 1, 2022

perrotta commented Dec 2, 2022

JetCorrector migration in DQM/Physics #39847

JetCorrector migration in DQM/Physics #39847

Conversation

wddgit commented Oct 25, 2022

PR description:

PR validation:

wddgit commented Oct 25, 2022

cmsbuild commented Oct 25, 2022

wddgit commented Oct 25, 2022

cmsbuild commented Oct 25, 2022

cmsbuild commented Oct 25, 2022

Comparison Summary

emanueleusai commented Oct 28, 2022

cmsbuild commented Oct 28, 2022

perrotta commented Oct 28, 2022

perrotta commented Oct 28, 2022

cmsbuild commented Oct 28, 2022

wddgit commented Nov 7, 2022

emanueleusai commented Nov 8, 2022

wddgit commented Nov 14, 2022

emanueleusai commented Nov 15, 2022

wddgit commented Nov 21, 2022

wddgit commented Nov 28, 2022

emanueleusai commented Nov 29, 2022

wddgit commented Nov 29, 2022

wddgit commented Dec 1, 2022

wddgit commented Dec 1, 2022

perrotta commented Dec 1, 2022

cmsbuild commented Dec 1, 2022

cmsbuild commented Dec 1, 2022

Comparison Summary

perrotta commented Dec 2, 2022