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

Fixup: Create some correction branches only for MC #253

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alpakpinar
Copy link
Contributor

@alpakpinar alpakpinar commented Oct 20, 2020

Hello, this PR provides a fix for an issue in fat jet JME module. With the changes in PR #251, FatJet_corr_JMR, FatJet_corr_JER and FatJet_corr_JMS branches are created for both data and MC, in [1] (unlike earlier versions, where they were only created for MC).

In our case, this difference caused a problem while running on data. Specifically the problem came from the execution of FatJet collectionMerger module, following the fatjet JME module, you can find the full traceback in [2]. I believe this issue is caused by these branches being created for data as well, in the version in this PR these three branches are created only for MC (like older versions) and we didn't encounter a problem running on data.

Please let me know if you want any changes to this, thanks so much!

[1] https://github.com/cms-nanoAOD/nanoAOD-tools/blob/master/python/postprocessing/modules/jme/fatJetUncertainties.py#L252-L260

[2]
Traceback (most recent call last):
File "crab_script_monojet.py", line 211, in
main()
File "crab_script_monojet.py", line 204, in main
p.run()
File "/uscms_data/d3/aakpinar/MYWORKINGAREA/CMSSW_10_2_15/python/PhysicsTools/NanoAODTools/postprocessing/framework/postprocessor.py", line 234, in run
eventRange=eventRange, maxEvents=self.maxEntries
File "/uscms_data/d3/aakpinar/MYWORKINGAREA/CMSSW_10_2_15/python/PhysicsTools/NanoAODTools/postprocessing/framework/eventloop.py", line 78, in eventLoop
ret = m.analyze(e)
File "/uscms_data/d3/aakpinar/MYWORKINGAREA/CMSSW_10_2_15/python/PhysicsTools/NanoAODTools/postprocessing/modules/common/collectionMerger.py", line 120, in analyze
out.append(getattr(obj, br) if self.is_there[bridx][j] else 0)
File "/uscms_data/d3/aakpinar/MYWORKINGAREA/CMSSW_10_2_15/python/PhysicsTools/NanoAODTools/postprocessing/framework/datamodel.py", line 69, in getattr
val = getattr(self._event, self._prefix + name)
File "/uscms_data/d3/aakpinar/MYWORKINGAREA/CMSSW_10_2_15/python/PhysicsTools/NanoAODTools/postprocessing/framework/datamodel.py", line 18, in getattr
return self._tree.readBranch(name)
File "/uscms_data/d3/aakpinar/MYWORKINGAREA/CMSSW_10_2_15/python/PhysicsTools/NanoAODTools/postprocessing/framework/treeReaderArrayTools.py", line 76, in readBranch
raise RuntimeError("Unknown branch %s" % branchName)
RuntimeError: Unknown branch FatJet_corr_JMR

@AndreasAlbert
Copy link
Contributor

Could you elaborate on how exactly this broke now? I looked at the git history but could not figure out what recent change would have made a difference here.

FWIW: I had previously proposed removing these branches in #202 but @fgolf convinced me to leave it for ease-of-use reasons. So in principle these branches have been produced for data for a long time, so it's not quite clear to me what changed here.

@alpakpinar
Copy link
Contributor Author

Hi, first of all, sorry about the wrong comparison I made, this property (the branches being created not only for MC) is not included in PR #251 , but it was there earlier as @AndreasAlbert pointed out.

But I think we have a better understanding of the problem now: In the central version the JMS,JER,JMR branches are initialized both in data and MC in 1. But then these are only filled for MC in 2. I believe this causes an issue with the following collectionMerger module: It sees that those branches are defined and looks for them, but cannot find anything and raises an error. If we are to create AND fill these branches (with 1s) for data, I encountered no such error.

So my understanding is we can either remove those branches for data completely, or can create them but make sure we fill them with 1s. What do you think? I can push some changes based on which way we would like to move on. Thanks a lot!

@alefisico
Copy link

Hi @alpakpinar, all
Thanks for the PR, but JME is in the process of having a central validation and "new features/bugs" implementation. Before that is completed, we prefer to NOT merge any new changes to our current tools. Is it possible if maybe you open an issue with your information and we can take care (if needed) later.
About your issue, I have personally run in the last days these modules and I didn't found any problem with them.
cc: @camclean

@alpakpinar
Copy link
Contributor Author

Hi @alefisico, thanks, yes sure I can open an issue about this, providing information about the error I encountered and we can discuss if something needs to be done. Thank you for investigating that, in the meantime I can double check the setup I'm running as well and try to see if something is not consistent with the setup itself. Thanks!

@alpakpinar
Copy link
Contributor Author

Hello, I recently opened an issue regarding this, #254, thanks a lot for looking into this!

@AndreasAlbert
Copy link
Contributor

About your issue, I have personally run in the last days these modules and I didn't found any problem with them.

Interesting! Do you mean you ran the same type of workflow, where you first run fatjet uncertainties and then subsequently run the collectionmerger on the new, modified fatjet collection?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants