Skip to content
Snippets Groups Projects

CP algs: add ability to disable PRW and just copy run/lumiblock number as random

Closed CP algs: add ability to disable PRW and just copy run/lumiblock number as random
tadej/athena:cp/prw into master
2 unresolved threads
Closed Tadej Novak requested to merge tadej/athena:cp/prw into master
2 unresolved threads

An alternative implementation of !52194 (closed). As we do not have Run 3 data yet we may want to fully disable the PRW in some cases and just copy run/lumiblock number as random. Fixes ATLASG-1628.

This method does not require any dummy files but PHYSLITE configuration is still hacked a bit. Probably longer term we should configure GRL/lumicalc files from the PHYSLITE configuration itself (cc @jcatmore).

/cc @will @zmarshal @krumnack @jburr

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
PhysicsAnalysis/Algorithms/AsgAnalysisAlgorithms/Root/PileupReweightingAlg.cxx
114 131 // Take care of the weight (which is the only thing depending on systematics)
115 for (const auto& sys : m_systematicsList.systematicsVector())
132 if (m_weightDecorator && !m_pileupReweightingTool.empty())
116 133 {
117 const xAOD::EventInfo* systEvtInfo = nullptr;
118 ANA_CHECK( m_eventInfoHandle.retrieve(systEvtInfo, sys));
119 ANA_CHECK (m_pileupReweightingTool->applySystematicVariation (sys));
120 if (m_weightDecorator)
134 for (const auto& sys : m_systematicsList.systematicsVector())
135 {
136 const xAOD::EventInfo* systEvtInfo = nullptr;
137 ANA_CHECK( m_eventInfoHandle.retrieve(systEvtInfo, sys));
138 ANA_CHECK (m_pileupReweightingTool->applySystematicVariation (sys));
121 139 // calculate and set the weight. The 'true' argument makes the tool treat unrepresented data
122 140 // correctly if the corresponding property is set
123 141 m_weightDecorator.set(*systEvtInfo, m_pileupReweightingTool->getCombinedWeight(*evtInfo, true), sys);
  • K
    Nils Erik Krumnack @krumnack ·
    Developer

    Shouldn't we at least allow to set a dummy weight here if the decorator is configured?

  • Tadej Novak
    Tadej Novak @tadej ·
    Author Developer

    I don't think so. Even now if one does not provide PRW configs the decoration just gets disabled and it's not set to some dummy value.

  • K
    Nils Erik Krumnack @krumnack ·
    Developer

    If somebody asks you to do something on an MR please don't just say "I don't think so" and close the discussion. In this case it was only one discussion and I did see the notification, but I had cases in which I had to expand dozens of resolved discussions to see for which ones I was actually happy with the reply.

    There are two points here:

    • My understanding of the purpose here is to provide the equivalent of providing a dummy PRW file so that downstream code remains oblivious to the missing PRW file. In my mind that would include the weight as well. Though, you are the more active user here, if your n-tuple doesn't contain PRW weight do your analysis macros still work or do you have to code a special handling in that case?
    • This would be the only CP algorithm that can have a decoration handle configured without setting the decoration. To the best of my knowledge all other CP algorithms will set a dummy value when no correct value is available.
  • Tadej Novak
    Tadej Novak @tadej ·
    Author Developer

    I do not like algorithms to be a black box. A decoration should be missing if it's not available and handled by the downstream code. More and more tools behave like this now. We also do not decorate SFs of 1 if a working point is not available (although we do it for AFII sometimes but CP groups do it on purpose).

  • K
    Nils Erik Krumnack @krumnack ·
    Developer

    Ok, if you feel strongly about not setting a dummy decoration here, please generate an error at this point if the user configured a decoration you do not feel you are able to set. As I said, all CP algorithms (so far) will set all the decorations configured, and we may want to rely on that at some point.

  • Z
    Zach Marshall @zmarshal ·
    Developer

    Hi Tadej,

    I agree with part of that principle, but maybe the disagreement is on what "not available" should mean. I would say that in this case we have a reasonable configuration of the PRW tool that can provide all the relevant information, it's just that it's an empty operation. A missing decoration (in my opinion) should imply that something didn't run or failed upstream, and the user should have to explicitly take some action. I'd want user code to not have to worry about the case that the pileup weight is 1, if the PRW tool was configured correctly (I think).

    Best, Zach

  • Tadej Novak
    Tadej Novak @tadej ·
    Author Developer

    OK an option is that if user provides a PRW config but not the lumicalc to just decorate 1 (we already disable the decorations if PRW config files are not provided).

  • Jon Burr
    Jon Burr @jburr ·
    Contributor

    I don't like the idea of a dummy decoration. It's far too easy for a user to see that and assume that the pileup reweighting was actually run. If we go this route I think we're fairly likely to see emails a few months/a year down the line from an analysis who just noticed that all their pileup weights were 1...

  • Please register or sign in to reply
  • Tadej Novak resolved all threads

    resolved all threads

    • Jon Burr
      Jon Burr @jburr ·
      Contributor

      Hmm - if the issue is that PRW cannot run yet, why not just tell the tools which depend on the RandomRunNumber not to use it? I'm pretty sure these all have properties that allow you to specify the 'random' run number to use instead - e.g. the AsgElectronEfficiencyCorrectionTool. That seems a better approach than allowing the PRW algorithm to not actually do any PRW...

      If that doesn't work, I'd prefer just creating an algorithm which can copy auxdata around generically than add behaviour like this (violating the S of SOLID)...

    • K
      Nils Erik Krumnack @krumnack ·
      Developer

      Personally, I'm not a fan of reconfiguring all the downstream code not to use the output of the PRW tool. If this was a permanent modus operandi I'd see the point, but for something that is essentially more of a test/trial configuration I'd rather replace the real data with mock data in one place and then just watch it propagate through the system in as much of a normal configuration as possible.

      That doesn't mean we have to modify the PileupReweightingAlg though, we could replace it with some other algorithm that provides the mock output. I don't really have a preference for one solution over the other.

    • Z
      Zach Marshall @zmarshal ·
      Developer

      Hi Nils,

      I tend to agree that we should use the PRWTool (or something like it) for this, and let it handle this aspect of the configuration in a single place. Just on the 'test configuration' point: I expect this to happen just about every year for the next four. We'll have some new MC with a pileup profile that doesn't correspond to any data, which is an attempt to model the coming year.

      Best, Zach

    • K
      Nils Erik Krumnack @krumnack ·
      Developer

      Hi @zmarshal,

      Ah, I wasn't aware that this would be a recurring issue. Somehow I assumed once we had data we would just reweight all MC to whatever the data profile was and keep updating that.

      Thanks, Nils

    • Tadej Novak
      Tadej Novak @tadej ·
      Author Developer

      Sure, we can modify the PRW tool to not randomise run numbers if no lumicalc files are provided, but I'm kind of against dummy files because this is the same level of hack than this one.

      What we should aim for is to be able to disable PRW without breaking downstream CP clients. Either if this is with a new algorithm or an existing one, it's fine for me.

      In any case I think we should simplify/modularise PRW tool for longer term.

      Just to add I'm on holidays from tomorrow on so feel free to combine the ideas that we have in a new MR.

    • Z
      Zach Marshall @zmarshal ·
      Developer

      Let me try this argument a different way. What we have now that we call the pileup reweighting tool in fact serves two purposes: 1) it reweights the amount of pileup in MC to the amount of pileup in the data (if there is data). 2) It provides an indication to all the downstream CP tools of what data taking period they should emulate. Even when we don't have data, the second functionality should be preserved (it matters if they are emulating 2022 or 2025 or 2030). I think the two operations are deeply related, so we don't want two different tools doing them, but I hope it makes more clear why I don't think providing a future run number is a hack or inappropriate.

      Best, Zach

    • Tadej Novak
      Tadej Novak @tadej ·
      Author Developer

      I agree that it can be one tool, but if there's no lumicalc we should not need to provide a dummy file. We should instead move the functionality I implemented here to the PRW tool itself. If no lumicalc just copy the run and LB number.

    • K
      Nils Erik Krumnack @krumnack ·
      Developer

      Doing this in the PRW tool would also seem like a reasonable option, maybe even better as it then benefits all frameworks.

    • Jon Burr
      Jon Burr @jburr ·
      Contributor

      OK - I'm clearly in the minority here so happy to accept this way of doing things. I do strongly agree with Tadej though that we should not solve this with a dummy data file. The properties of tools should be explicit about what they're doing. If we want the PRW tool to set a non-random run number, then we should add a property that tells it to do just this. It shouldn't just decide to do that if the user provides a specific lumicalc file, or even if the user provides no lumicalc files (most of the time this will just be user error). We shouldn't have tools having to guess what the user means.

    • Please register or sign in to reply
  • Nils Erik Krumnack added analysis-review-expert label and removed analysis-review-approved label

    added analysis-review-expert label and removed analysis-review-approved label

  • Carlo Varni
    Carlo Varni @cvarni ·
    Developer

    Changes look good from a L1 perspective. Setting review-user-action-required while open threads are being resolved.

    -- L1

  • Carlo Varni added review-user-action-required label and removed review-pending-level-1 label

    added review-user-action-required label and removed review-pending-level-1 label

  • Jason Robert Veatch
    Jason Robert Veatch @jveatch ·
    Developer

    Is there any news on resolving the open threads?

    Jason (L1)

  • K
    Nils Erik Krumnack @krumnack ·
    Developer

    Hi @tadej,

    Is this MR still relevant?

    Regards, Nils

  • Tadej Novak
    Tadej Novak @tadej ·
    Author Developer

    No, there's an alternative here: !55377 (merged)

  • Tadej Novak closed

    closed

    • Please register or sign in to reply