-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 bug in the jetIDcleaning module #12346
Conversation
A new Pull Request was created by @safarzad for CMSSW_8_0_X. fix bug in the jetIDcleaning module It involves the following packages: HLTrigger/JetMET @Martin-Grunewald, @perrotta, @cmsbuild, @fwyzard can you please review it and eventually sign? Thanks. |
Pull request #12346 was updated. @Martin-Grunewald, @perrotta, @cmsbuild, @fwyzard can you please check and sign again. |
I made this PR from CMSSW800_pre1 release hope works |
You still did not remove "usePt" from the .h header file... |
Pull request #12346 was updated. @Martin-Grunewald, @perrotta, @cmsbuild, @fwyzard can you please check and sign again. |
please test |
The tests are being triggered in jenkins. |
reco::CaloMET cleanmet = met->front(); | ||
cleanmet.setP4(cleanmet.p4() + p4_diff); | ||
reco::CaloMET cleanmet ; | ||
cleanmet.setP4(p4_diff); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Batool,
thanks for putting up the PR!
Two questions/suggestions came to my mind after taking a look at the proposed changes:
- what about renaming p4_diff to e.g. cleanmetp4 (L96/99)? With the proposed code changes, it has a different meaning than before. It would be good to change the name, then.
- Does the change from reco::CaloMET cleanmet = met->front(); to initializing a fresh object reco::CaloMET cleanmet ; before setting the p4 loose any information that might be useful to have? The way you propose is definitely cleaner, I am just wondering...
Cheers,
Henning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the proponent please address these issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
I do not see any issue here, it is about renaming a collection that I do not think make any differences at the end in physiscs.
The second part is not clear for me which information will be missing, but in general the idea here is to have a new collection that does not inherent from original caloMET collection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Is there anything left that I must to do to merge this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kirschen
please comment!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, my comments are no showstoppers, but I thought we could follow up quickly. To clarify a bit what I was aiming at:
- Renaming:
Sure, it does not change physics, but why should one retain misleading/wrong variable names? - Usability of cleanmet:
If I understand correctly, the "cleanmet" here will contain only the p4-vector information, but none of the CaloMET-specifics that are usually added in https://github.com/cms-sw/cmssw/blob/6b16de370881dd8ef339d34811b3d1e176c02b80/RecoMET/METAlgorithms/src/CaloSpecificAlgo.cc called from https://github.com/cms-sw/cmssw/blob/6b16de370881dd8ef339d34811b3d1e176c02b80/RecoMET/METProducers/src/CaloMETProducer.cc
So the output here uses the CaloMET data format, but only contains a small fraction of the information that is usually associated with this object. However, I guess putting the minimal information here is still better than the old behavior before the PR (retaining the old detail information before JetId-cleaning, which is incorrect when removing jets).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Martin,
If I make a small modification from p4_diff to p4_Clean do we need to go trough the whole test procedures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please test |
The tests are being triggered in jenkins. |
-1 runTheMatrix-results/1001.0_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4/step3_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4.log you can see the results of the tests here: |
To find out what is the error I checked this (i think it is that you are referring right?) but there is no error just processing the event. Can you clarify? |
To find out what is the error I checked this (i think it is that you are referring right?) but there is no error just processing the event. Can you clarify? From: cmsbuild [notifications@github.com] -1 runTheMatrix-results/1001.0_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4/step3_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4.log you can see the results of the tests here: — |
The log file seems cut off (the usual end of run messages are missing). |
please test |
The tests are being triggered in jenkins. |
+1 |
This pull request is fully signed and it will be integrated in one of the next CMSSW_8_0_X IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @slava77, @davidlange6, @Degano, @smuzaffar |
+1 |
fix bug in the jetIDcleaning module
fix bug in the jetIDcleaning module