search

TopEFT / topeft

15 stars 24 forks source link

Loading systematics weights when processing nominal #232

Open bryates opened 2 years ago

bryates commented 2 years ago

The topeft.py processor seems to load some of the systematic weights regardless of self._do_systematics: https://github.com/TopEFT/topcoffea/blob/9d6a2148f5fdf39ad4f2e7c1660657ba0740e47f/analysis/topEFT/topeft.py#L227-L238 We should wrap this section, and any others, inside if self._do_systematics:

kmohrman commented 2 years ago

@bryates, I actually think the code is correct, it's just inefficient. We always need the nominal variations of the SFs, which we store using the Weights object, so we cannot wrap that section (and other similar sections) in if self._do_systematics. The Weights object, however, also stores our up/down fluctuations. We do not access or apply the up/down fluctuations when running with the systematics turned off, so it's technically not necessary to store them in the Weights object in this case. To make this more efficient, we'd need a separate Weights object that we use in the case of systematics (where we also pass the up/down fluctuations to add()) and in the case of no systematics (where we would only pass the nominal), or a lot of if statements. However, without rewriting the entire processor, I think a fix for this would be really messy and involve a lot of if statements that would make the code harder to follow. Furthermore, the change would only speed up the processing in the case where we are not including systematics (which is already fairly quick anyway).

For these reasons, I don't think this is actually a bug, I think it's just something that could be potentially optimized. However, I am not sure this is something we have time to prioritize right now, especially since it's not clear that we'd gain a whole lot from the optimization (plus we're usually running with systematics on right now anyway, and we'd gain nothing in that case).

kmohrman commented 2 years ago

For these reasons, I'd also probably vote for removing the "needs to be addressed" label from this issue (since I'm not sure it's something we need to address before finalizing the analysis). But of course feel free to let me know if you disagree.

bryates commented 2 years ago

Ok, I agree with removing the label. The parton shower and renorm/fact I believe just use one for the nominal, so we could move them. We should check the pre fire as well. I agree it's not really a bug, just inefficient.