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

Herwig7 jet partonFlavour (10_6_X) #39403

Merged
merged 2 commits into from Nov 25, 2022

Conversation

mseidel42
Copy link
Contributor

PR description:

Backport of #26885

Needed for jet flavor correction / uncertainty studies to distinguish light quark and gluon jets. Will ask for new 10_6 release and reprocessing of Herwig7 JMENanos afterwards.

Note that will also fix the issue for any newly produced MiniAods.

PR validation:

Jet_partonFlavour becomes available in TT_TuneCH3_13TeV-powheg-herwig7 JMENano after applying patches:

 Jet_hadronFlavour = 0,                                                                                                                                                                                     
                  4, 5, 5, 0, 0, 0, 0, 0, 0, 0,                                                                                                                                                             
                  0, 0, 0, 0, 0, 0, 0, 0, 0                                                                                                                                                                 
 Jet_partonFlavour = -2,                                                                                                                                                                                    
                  4, 5, -5, 21, -3, 0, 1, 21, 0, -2,                                                                                                                                                        
                  0, 0, 21, 0, 21, 21, 0, 0, 2

@cms-sw/jetmet-pog-l2 FYI

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 15, 2022

A new Pull Request was created by @mseidel42 (Markus Seidel) for CMSSW_10_6_X.

It involves the following packages:

  • PhysicsTools/JetMCAlgos (analysis)

@cmsbuild can you please review it and eventually sign? Thanks.
@AlexDeMoor, @emilbols, @AnnikaStein, @JyothsnaKomaragiri, @andrzejnovak, @demuller 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

@perrotta
Copy link
Contributor

backport of #26885

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce5675/27733/summary.html
COMMIT: e16c421
CMSSW: CMSSW_10_6_X_2022-09-18-0000/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39403/27733/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

The workflows 140.53 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 1074 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 3215686
  • DQMHistoTests: Total failures: 2154
  • DQMHistoTests: Total nulls: 105
  • DQMHistoTests: Total successes: 3213093
  • DQMHistoTests: Total skipped: 334
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -81.002 KiB( 34 files compared)
  • DQMHistoSizes: changed ( 140.53 ): -63.680 KiB Hcal/DigiRunHarvesting
  • DQMHistoSizes: changed ( 140.53 ): -15.641 KiB Info/EventInfo
  • DQMHistoSizes: changed ( 140.53 ): -1.676 KiB RPC/DCSInfo
  • DQMHistoSizes: changed ( 140.53 ): -0.006 KiB SiStrip/MechanicalView
  • Checked 143 log files, 29 edm output root files, 35 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

assign generators
Please @cms-sw/generators-l2 confirm with your signature that this PR, originally merged in master more that three years ago, is now also needed in 10_6_X

@cmsbuild
Copy link
Contributor

New categories assigned: generators

@mkirsano,@menglu21,@alberto-sanchez,@SiewYan,@GurpreetSinghChahal,@Saptaparna you have been requested to review this Pull request/Issue and eventually sign? Thanks

@Saptaparna
Copy link
Contributor

+1

@perrotta
Copy link
Contributor

perrotta commented Oct 4, 2022

please test
(to refresh the comparisons, as it is not clear to me why there should be differences in the HI workflows)

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 4, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce5675/27977/summary.html
COMMIT: e16c421
CMSSW: CMSSW_10_6_X_2022-10-02-0000/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39403/27977/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: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 3215686
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3215350
  • DQMHistoTests: Total skipped: 334
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 143 log files, 29 edm output root files, 35 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

perrotta commented Oct 4, 2022

This changes the parton content in HI workflows, which is something that should be avoided in closed releases:
image

Perhaps, a modifier would allow maintaining separate the productions with and without this feature.

This can be discussed in the ORP meeting in half an hour from now

@Saptaparna
Copy link
Contributor

Let me add our HERWIG experts @Dominic-Stafford and @theofil to this thread.

@Saptaparna
Copy link
Contributor

@mseidel42 this is what is expected right (reference to Andrea's message)? I am fine with merging, but let's double check just in case.

@Dominic-Stafford
Copy link
Contributor

@perrotta do you have a link to the ORP meeting, please?

@Dominic-Stafford
Copy link
Contributor

But in general I agree with Sapta, I don't see an issue with changing the parton content here since it's just adding information that wasn't there before

@Saptaparna
Copy link
Contributor

here it is: https://indico.cern.ch/event/1208161/

@mseidel42
Copy link
Contributor Author

Hi all, I need to have a closer look at the HydJet event record.

If blue=old and red=new then things would make sense: previously missed gluons are now taken into account for the jet flavor.

@perrotta
Copy link
Contributor

perrotta commented Oct 4, 2022

As discussed at the ORP meeting right now, the issue is not whether the change makes sense or not, but rather whether we can modify GenJets in some frozen produced release. It was agreed with @Dominic-Stafford that the update will be ruled by a modifyer, so that only the dedicately produced MC samples will have it included

@mseidel42
Copy link
Contributor Author

Hi, that's an overengineered and error-prone (because it requires manual intervention) solution to a non-problem. We are just fixing partonFlavour for samples where it was broken and completely unusable before. If anybody was actually using those 0 values they need to repeat their study with a fixed sample.

If you prefer I can revert e16c421 and just leave the minimally invasive bbfc5d1, fixing the issue for Herwig7 samples (that we are interested in for jet flavor studies that can just not be done with the buggy samples) but leaving other "exotic" samples with the bug.

@mseidel42
Copy link
Contributor Author

mseidel42 commented Oct 5, 2022

For a non-code solution we could just close this PR and run the specific requests with --customise PhysicsTools/NanoAOD/custom_jme_cff.PrepJMECustomNanoAOD_MC --customise_commands process.patJetPartonsRecluster.partonMode=\"Herwig++\". The JME modifier is needed in any case as we need JMENano, which overwrites the flavor information from MiniAod.

@rappoccio
Copy link
Contributor

rappoccio commented Nov 15, 2022

removed "+1" , see below

Per discussion at ORP, PDMV agrees to document future workflows appropriately with the bug fix, agreed at the meeting to merge.

@perrotta
Copy link
Contributor

Should this get closed in favor of #39971? Once fixed that PR would be the cleaner solution.

Still I don't know whether any different decisione was decided at the ORP on nov 15 , at which I was unable to participate.

@rappoccio @cms-sw/pdmv-l2 could you please summarize it here, so that we can close the unneeded PR?

@mseidel42
Copy link
Contributor Author

Hi Andrea,

On 15 Nov we agreed that this PR would be merged and new samples with the bugfix marked by PdmV.

This would have the advantage that all new samples with Herwig7 (which was missed) and non-standard generators will finally have a meaningful partonFlavour for the jets. We would also have agreement again with the default behavior in both Run 1 and Run 3 production.

As long as 10_6 is relevant for Run 2 analysis I really don't see any value in producing new samples without the proper flavor information. The information is also usually not used in analysis, so it should not break anyone's workflow, while enabling us to do precision studies on jet flavor response.

Best,
Markus

@perrotta
Copy link
Contributor

Dear Markus, thank you for your message.
Unfortunately I could not be present at the ORP meeting of nov 15. However, I read in the minutes that the agreement was exactly the one that you are reporting here, therefore we will likely merge this PR and cut the release with it.

However; I think this is a wrong decision, and I would like to ask explicitely the confirmation by @cms-sw/pdmv-l2 .
It is wrong exactly because of what you say: samples with correct and wrong hervig should be made easily distinguishable, and the modifier (which can be applied to all productions in 10_6 from now on) has exactly that purpose.

In any case, I don't want to revert any decision taken. Therefore, if @cms-sw/pdmv-l2 confirms that this is really what they plan for the 10_6 cycle, I'll merge this PR and make a new release with it. (Otherwise, we will merge #39971 and make a release with it: therefore, the release with the fix will become available anyhow in the next few days).

@bbilin
Copy link
Contributor

bbilin commented Nov 24, 2022

Dear Andrea,

Given that we need a new chained campaign for Herwig samples anyways to be able to do JMENano, and provided these will affect only few samples and not affecting anything else, we can handle it internally in PdmV management to add a bugfix tag to the new Herwig samples.

Best,

B.

@perrotta
Copy link
Contributor

please test
(to refresh the more than one month old tests)

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ce5675/29246/summary.html
COMMIT: e16c421
CMSSW: CMSSW_10_6_X_2022-11-20-2300/slc7_amd64_gcc700
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/39403/29246/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: 35
  • DQMHistoTests: Total histograms compared: 3215686
  • DQMHistoTests: Total failures: 3
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3215349
  • DQMHistoTests: Total skipped: 334
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 143 log files, 29 edm output root files, 35 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

+1

  • As deemed acceptable by PPD

@perrotta
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 6ef5341 into cms-sw:CMSSW_10_6_X Nov 25, 2022
@perrotta perrotta mentioned this pull request Nov 25, 2022
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

8 participants