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

WIP: Improved parsing... more improved! #13

Merged
merged 7 commits into from
May 28, 2020

Conversation

dteague
Copy link

@dteague dteague commented May 21, 2020

PR description:

Fixing final issues with the parsing code. Here is a checklist of what this PR will hope to do:

  • Fix parsing issues where parsing breaks
  • Check that all LHE test files are parsed in the desired way
  • Check the parsing code running over miniAOD -> NanoAOD

Limitations:

There are some noticed problems.

  • For Scale weights, the first (central) scale weight is sometimes not included in the "scaleWeight" group. This happens often enough that it might be worth it to implement a fix
<weightgroup name="NNPDF31_nnlo_hessian_pdfas" combine="symmhessian+as"> 
    <weight id="1001" MUR="1.0" MUF="1.0" PDF="306000" >  </weight>
</weightgroup> 
<weightgroup name="Central scale variation" combine="envelope">
    <weight id="1002" MUR="2.0" MUF="1.0" PDF="306000" > MUR=2.0  </weight>
    <weight id="1003" MUR="0.5" MUF="1.0" PDF="306000" > MUR=0.5  </weight>
    <weight id="1004" MUR="1.0" MUF="2.0" PDF="306000" > MUF=2.0  </weight>
    <weight id="1005" MUR="2.0" MUF="2.0" PDF="306000" > MUR=2.0 MUF=2.0  </weight>
    <weight id="1006" MUR="0.5" MUF="2.0" PDF="306000" > MUR=0.5 MUF=2.0  </weight>
    <weight id="1007" MUR="1.0" MUF="0.5" PDF="306000" > MUF=0.5  </weight>
    <weight id="1008" MUR="2.0" MUF="0.5" PDF="306000" > MUR=2.0 MUF=0.5  </weight>
    <weight id="1009" MUR="0.5" MUF="0.5" PDF="306000" > MUR=0.5 MUF=0.5  </weight>
</weightgroup> 
  • For scale weights, currently DYN_Scale weights are totally ignored. It would be nice to work towards adding that, but input would be good
  • The current code uses LHAPDF library to get LHA info, but this doesn't grab uncertainty type. Right now, the code sets all the pdf uncertainties to kUnknownUnc.
  • There are certain fixes put in place to handle specific errors in the test lhe files that will become outdated as the xml is cleaned before or standardized, and it could cause unintended behavior as more exotic xml files pop up. For instance, in one file, the group weights have .LHgrid appended to the pdfset name, so MMHT2014lo68cl.LHgrid is the seen pdf weight which can't be found by the pdfsetinfo. The current fix is to remove the substring after the ., but that could cause errors for groupnames that are supposed to have a . in them

those files being "DrellYan_LO_MGMLMv233_2016_weightInfo.txt" and "DrellYan_NLO_MGFXFXv242_2017_weightInfo.txt",
@kdlong
Copy link
Owner

kdlong commented May 21, 2020

Very good! I will look at this a bit this afternoon. A few quick comments:

  1. I think in cases where the central weight is by itself, we can either keep it a separate group, but handle it properly at the NanoAOD stage, or merge it with the other scale weights. The latter would be easier to handle in ntuples, for example, but the former might be easier to code. We can think about it.

  2. Yes, the dynamic scale weights should be handled somehow. We don't necessarily need to split the group but they should be distinguishable from the other weights.

  3. Probably removing the period is ok, you could also remove the full ".LHgrid." I think this is the name of the PDF grid file rather than the PDF set.

Dylan Teague added 2 commits May 21, 2020 15:59
May put flags needed for the splitting. Only impliemented in LHEWeightHelper
@dteague
Copy link
Author

dteague commented May 21, 2020

I will try to address point 1. and 2. in future commits, and for 3., i'll just keep the code as is with the removing things after the period

I refactored the code some to streamline things so that the PDFweights are always split. I have it working and it's shrunk the code some, but it'll probably be useful to have helper functions to assist with the automatic splitting, especially if the PDFweights are going going be used outside the LHEWeightHelper file...

I'm also thinking of adding in more consistency checking. PdfWeights now can be added, but sometimes the number of members doesn't match up with the expect number for a given PDFSet. Most of the cases are just one entry in the PDFSet (which I assume is ok since it's the central value, or?), but this might be good practice anyway to have this check being done

@kdlong
Copy link
Owner

kdlong commented May 21, 2020

Very good. Can you give some examples of the set numbers not matching?

I've started testing samples from DAS and cateloguing the results here: https://docs.google.com/spreadsheets/d/1Qlg1c6qrMGY0q111Va2Vl0fJQUz15ZTjKEvRaV8rHaQ/edit?usp=sharing

I'm generate the config with the cmsDriver command in this script: https://github.com/kdlong/WMassNanoGen/blob/master/runCmsDriverGenericMiniToNanoGen.sh

Then testing the info that gets written to Nano with this: https://github.com/kdlong/cmssw/blob/ImprovedParsing/PhysicsTools/NanoAOD/test/testNanoWeights.py

You can also get the MadGraph version from Nano (or Mini) with this script: https://github.com/kdlong/TheoreticalUncertainties/blob/master/getGridpackFromProv.sh

I sent an email to the gen conveners about having some help testing this, hopefully there's a student who can help.

@kdlong
Copy link
Owner

kdlong commented May 28, 2020

This is a pain for me to keep monitoring while adding my own changes, so I'm going to merge. You can create an issue with the description if you'd like

@kdlong kdlong merged commit 6cd57c6 into kdlong:ImprovedParsing May 28, 2020
kdlong pushed a commit that referenced this pull request Jun 19, 2020
…ms (L1Trigger/TrackFindingTMTT) (cms-sw#29381)

* create separate PRs for the two L1TK packages

* Improved KF efficiency at high eta

* Moved MC data files to cms-data

* Removed old file

* Removed KF HLS to put instead in external library

* Ran scram b code-format

* Delete KF4ParamsComb.h.bak

* Delete KF4ParamsCombIV.bak

* Delete KF4ParamsCombV2.bak

* Delete KF5ParamsComb.h.bak

* Delete KF4ParamsComb.cc.bak

* Delete KF4ParamsCombIV.bak

* Delete KF4ParamsCombV2.bak

* Delete KF5ParamsComb.cc.bak

* L1 tk integration tmtt pre5 (#7)

* Added CMS code style fixes

* Removed old file

* Reapplied stub b code-format

* All code review changes (#13)

* Fix clang errors (#14)

* fixed clang error

* directory for MC txt files

* Fixed clang warnings + minor simplifications (#15)

* tweak

* tweak

* Fixed clang warnings and small simplifications

* Fixed clang warnings and small simplifications

* All remaining review comments addressed (#16)

* Replaced vector size with empty function

* Simplified DegradeBend and StubWindowSuggest

* Fixed more review comments

* More review comments

* code reformat

* Ran exhaustive clang tidy

* Added library to BuildFile.xml (#17)

* Deleted TrackFindingTMT/data/README (#18)

* Added library to BuildFile.xml (This was already done yesterday. Not sure why it appears again)

* README file in data directory deleted

* Fix review comments (#20)

Co-authored-by: Louise Skinnari <louise.skinnari@cern.ch>
kdlong pushed a commit that referenced this pull request Jan 27, 2021
Modifying pepr PF candidate producer to use new Triton client in CMSSW
kdlong pushed a commit that referenced this pull request Jan 6, 2022
kdlong added a commit that referenced this pull request Jun 14, 2022
Copy in all the recent nano weights stuff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants