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

Fixes to auto PRW #1682

Merged
merged 7 commits into from Mar 27, 2024
Merged

Fixes to auto PRW #1682

merged 7 commits into from Mar 27, 2024

Conversation

SagarA17
Copy link
Contributor

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
Copy link
Contributor

mdhank commented Mar 26, 2024

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
Copy link
Contributor Author

SagarA17 commented Mar 26, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

mdhank commented Mar 27, 2024

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 mdhank merged commit 8bc0c28 into UCATLAS:main Mar 27, 2024
12 checks passed
@mdhank
Copy link
Contributor

mdhank commented Mar 27, 2024

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 SagarA17 deleted the fixAutoPRW branch March 27, 2024 18:41
@SagarA17
Copy link
Contributor Author

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
Copy link
Contributor

mdhank commented Mar 27, 2024

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

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.

None yet

2 participants