search

UCATLAS / xAODAnaHelpers

ATLAS Run 2 and Run 3 analysis framework for AnalysisTop and AnalysisBase for proton-smashing physics
https://ucatlas.github.io/xAODAnaHelpers/
Apache License 2.0
42 stars 127 forks source link

Fixes to auto PRW #1682

Closed SagarA17 closed 8 months ago

SagarA17 commented 8 months ago

This MR adds tiny fixes to autoconfigurePileupRWTool to make the common PRW file work. Nothing major. I also took the liberty to clean up indentation which adds to the readability of the code. And as usual, updated the ci to show the newer releases.

Urging to merge ASAP since this is a bug fix.

mdhank commented 8 months ago

Hi @SagarA17 ,

Could you ping me know when you're done updating this? I see you've been adding updates, and wasn't sure if you have more incoming or not.

Could you also specify what behaviors are changed? It's a difficult to tell from the diff given the indentation changes. So far I see L1178, L1270-1275, and the top_CMakeLists.

Was the common PRW failing for you before? I'd be interested to hear how/when it stopped working.

Thanks, Michael

SagarA17 commented 8 months ago

Hi @mdhank,

I am done with my updates :) There was some tinkering to make the ci work for 25.2.X releases, but now it's stable. The behavioral changes are in -

  1. L1178: The metadata was somehow not able to resolve the mcCampaign (perhaps due to derivation version? not sure) from the input file. The runNum scope worked well, but then the mcCampaignMD variable was being overwritten by an empty string. I have fixed this behavior in L1178, where on top of the standard MetaInput check, the code also checks to see if mcCampaign is available in the input file.
  2. L1270-1275: I believe this was a bug. Given that the running variable of the loop is mcCampaign, the check to obtain the commonPRW should also be based on mcCampaign. Forcing the if-else checks to use mcCampaignMD rendered the manual input of the m_mcCampaign flag redundant. Furthermore, because of the upstream issue in mcCampaignMD, the commonPRW scope would always fall into the error case. After this fix, we shouldn't face any errors.
  3. I had to change a line in the top level CMakeLists.txt to add support to 25.2.x releases, which wasn't possible with the hard requirement on the version to be 24.2.

This should be all!

Cheers, Sagar

SagarA17 commented 8 months ago

Hi @mdhank,

And about the common PRW failing before: I have never tried it before! I wanted to make use of it today and ran into these issues. The auto PRW (based on DSID and campaign and sim flavor) has worked fine for me in the past.

Cheers, Sagar

SagarA17 commented 8 months ago

Hi @mdhank, any updates on this? It would be helpful to merge this soon if you are satisfied with the changes. The alternative would be install the NTUP_PILEUP to cvmfs, which is not preferred by ATLAS anymore.

mdhank commented 8 months ago

Hi @SagarA17 ,

Barring any unforeseen issues, I should be able to get to this today or tomorrow. I agree the changes look reasonable, I just want to run a couple quick tests to confirm the before/after behavior.

Best, Michael

mdhank commented 8 months ago

Hi @SagarA17 ,

I merged this, but could you let me know what file/derivation version you were using? I was able to get it to resolve the campaign before the changes, on a p5855 DAOD_PHYS file.

Thanks for catching the L1270-1275 bug! I hadn't provided a user-input mcCampaign in my original tests, so hadn't noticed this.

Best, Michael

SagarA17 commented 8 months ago

Hi @mdhank, Thanks very much for merging! I used a p5771 DAOD_LLP1. You can try this one - mc20_13TeV.525013.MGPy8EG_A14N23LO_HNL2_ctau100_mumupi_LO.deriv.DAOD_LLP1.e8555_a907_r14861_p5771.

Cheers, Sagar

mdhank commented 8 months ago

Hi @SagarA17 ,

I can confirm I see the same issue in my setup, so I suspect it's an issue with the derivation.

Best, Michael