-
Notifications
You must be signed in to change notification settings - Fork 126
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
Fixes to auto PRW #1682
Conversation
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, |
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.
This should be all! Cheers, |
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, |
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. |
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, |
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, |
Hi @mdhank, Cheers, |
Hi @SagarA17 , I can confirm I see the same issue in my setup, so I suspect it's an issue with the derivation. Best, |
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.