Final tweaks for the MCP winter recommendations

Hi everybody,

I open this MR to get the final tweaks needed for the winter update of the MCP winter recommendations into 21.2 following up !20096 (merged). The changes include

• Update to the latest calibration area of MuonEfficiencyCorrections
• Exchange the property useLowPtThreshold - which is an expert property and no user should touch it - by useLowPtMap
• That change is needed because the isolation scale-factors also now include a low-pt map for muons between 3-4 GeV which is totally different from the used threshold of 15 GeV for Zmumu reconstruction. To prevent the user of making any real dull mistake the property is now boolean and the thresholds can no longer be changed.

Adding @szambito, @fsforza, @nkoehler and the guard of athena @atlasbot

added 27 commits

• 531aeebe...750742dd - 26 commits from branch atlas:21.2
• b7c5bc60 - Merge branch '21.2' of ssh://gitlab.cern.ch:7999/atlas/athena into MCP_WinterRecom

Compare with previous version

added 35 commits

• b7c5bc60...4c1a44db - 34 commits from branch atlas:21.2
• 9e0b25b8 - Merge branch '21.2' of ssh://gitlab.cern.ch:7999/atlas/athena into MCP_WinterRecom

Compare with previous version

added 1 commit

• eaa7efa1 - Switching to the new calibration area

Compare with previous version

changed the description

unmarked as a Work In Progress

This merge request affects 1 package:

• PhysicsAnalysis/MuonID/MuonIDAnalysis/MuonEfficiencyCorrections

Adding @morgens ,@nkoehler ,@jojungge as watchers

added 21.2 Analysis review-pending-level-1 labels

added Recommendation urgent labels

added MuonSpectrometer label

marked as a Work In Progress

CI Result FAILURE

	AnalysisBase	AnalysisTop	AthAnalysis	AthDerivation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-20869-2019-02-06-18-26
AnalysisBase: number of compilation errors 0, warnings 9
AnalysisTop: number of compilation errors 0, warnings 9
AthAnalysis: number of compilation errors 0, warnings 10
AthDerivation: number of compilation errors 0, warnings 54
For experts only: Jenkins output [CI-MERGE-REQUEST 33430]

added 1 commit

• dd8a22c6 - Isolation scale-factors between 3&4 GeV are taken from the low-pt sclae-factor map

Compare with previous version

changed the description

added 1 commit

• 1b499c9c - Refined the file return logic

Compare with previous version

unmarked as a Work In Progress

This merge request affects 1 package:

• PhysicsAnalysis/MuonID/MuonIDAnalysis/MuonEfficiencyCorrections

Adding @morgens ,@nkoehler ,@jojungge as watchers

changed the description

This looks ok, I think, so I'm ok to approve obo L1 to get these recommendation updates in for tomorrow's release build.

However, I note that the code is quite messy and could benefit from some clean-up. For example, float MuonEfficiencyScaleFactors::lowPtTransition() seems to return a pT threshold - more descriptive naming wouldn't hurt. I also see that in several places variable and function names don't follow the ATLAS conventions for capitalization, so it would be good to fix this for future updates (clearly we shouldn't hold up any recommendation update because of this).

Best, Christian

added review-approved label and removed review-pending-level-1 label

enabled an automatic merge when the pipeline for 1b499c9c succeeds

CI Result FAILURE

	AnalysisBase	AnalysisTop	AthAnalysis	AthDerivation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-20869-2019-02-06-22-10
AnalysisBase: number of compilation errors 0, warnings 1
AnalysisTop: number of compilation errors 0, warnings 1
AthAnalysis: number of compilation errors 0, warnings 1
AthDerivation: number of compilation errors 0, warnings 2
For experts only: Jenkins output [CI-MERGE-REQUEST 33463]

Looks like problems with calibration files:

http://atlas-computing.web.cern.ch/atlas-computing/links/distDirectory/ci/CIWebArea/nicos_web_areaMRCIbuilds64BS6G62AnalysisTopOpt/NICOS_TestLog_MR-20869-2019-02-06-22-10/ReleaseTests___TopXAOD-test__TopXAOD-test__m.html

Weirdly only for AnalysisTop, for MuonEfficiencyCorrections/190203_Winter_r21/Reco_Medium_JPsi.root.

Best, Christian

They should go away once /eos/atlas/atlascerngroupdisk/asg-calib/MuonEfficiencyCorrections/190203_Winter_r21/ is mirrored to cvmfs (e.g. that specific file is in eos already but not yet in cvmfs)

Jenkins please retry a build

This merge request affects 1 package:

• PhysicsAnalysis/MuonID/MuonIDAnalysis/MuonEfficiencyCorrections

Adding @morgens ,@nkoehler ,@jojungge as watchers

added review-pending-level-1 label and removed review-approved label

CI Result FAILURE

	AnalysisBase	AnalysisTop	AthAnalysis	AthDerivation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-20869-2019-02-07-01-54
AnalysisBase: number of compilation errors 0, warnings 0
AnalysisTop: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
AthDerivation: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST 33467]

Jenkins please retry a build

This merge request affects 1 package:

• PhysicsAnalysis/MuonID/MuonIDAnalysis/MuonEfficiencyCorrections

Adding @morgens ,@nkoehler ,@jojungge as watchers

CI Result FAILURE

	AnalysisBase	AnalysisTop	AthAnalysis	AthDerivation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-20869-2019-02-07-08-04
AnalysisBase: number of compilation errors 0, warnings 0
AnalysisTop: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
AthDerivation: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST 33469]

Jenkins please retry a build

This merge request affects 1 package:

• PhysicsAnalysis/MuonID/MuonIDAnalysis/MuonEfficiencyCorrections

Adding @morgens ,@nkoehler ,@jojungge as watchers

hi @pberta , all,

thanks for re-submitting the build and for having a close look to this, I fear that we have to give one more pass to the files on cvmfs and to the configurations loading.

Sorry for the spam...

CI Result FAILURE

	AnalysisBase	AnalysisTop	AthAnalysis	AthDerivation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-20869-2019-02-07-10-08
AnalysisBase: number of compilation errors 0, warnings 0
AnalysisTop: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 1
AthDerivation: number of compilation errors 0, warnings 1
For experts only: Jenkins output [CI-MERGE-REQUEST 33471]

Hi @fsforza,

I checked and I saw some updated files on cvmfs. Is it still necessary to wait for another update?

L1 shifter

Hi @pberta, the failing test from AnalysisTop is due to the fact, that the test tries to use the GradientIso muon isolation working point (http://atlas-computing.web.cern.ch/atlas-computing/links/distDirectory/ci/CIWebArea/nicos_web_areaMRCIbuilds64BS6G62AnalysisTopOpt/NICOS_TestLog_MR-20869-2019-02-07-10-08/ReleaseTests___TopXAOD-test__TopXAOD-test__m.html). To my knowledge (but I let @maklein comment), this working point is deprecated and therefore no _DrellYan file will be available. In my opinion, the test should use another working point which is supported. @maklein, can you comment on that? Best, Nico

Yes, you speak the truth. Note that this would also apply to FixedCutTight, FixedCutLoose, FixedCutTightTrackOnly, GradientLoose, Loose, and LooseTrackOnly.

Thanks @maklein for this confirmation. Could you give the people from AnalysisTop a "good advice" which working points they could check in their tests? Many thanks, Nico

The defined WPs are FCLoose, FCLoose_FixedRad, FCTight, FCTight_FixedRad, FCTightTrackOnly, FCTightTrackOnly_FixedRad, FixedCutPflowLoose, and FixedCutPflowTight

Tagging @mvanadia, @iconnell, @smergelm

Hi

I've not been following the IFF, but this means that the experimental working points are now the recommended ones? Is there a recommended replacement for gradient isolation with the pileup robust isolation variables? I couldn't see a comparison of the new and old isolation efficiencies. I note the pflow one sounds like it would be useful, but there is not a comment on the IFF twiki stating which derivation cache the new variables were added to the smart slimming lists which would be useful to know (especially for data derivations).

I think to fix this, the file PhysicsAnalysis/TopPhys/xAOD/TopAnalysis/share/validation-cuts.txt needs to be updated so that the line MuonIsolation Gradient is updated to MuonIsolation FCTight_FixedRad (assuming this is close in performance to Gradient). I will tag @spalazzo, @tdado and @pjacka as there might need to be some cleaning up in TopConfig and TopCPTools depending on whether this fix allows this MR to proceed.

Ian

Hi @iconnell,

a list of recommended and supported muon isolation working points is for example here. The changes have been advertised since a while in MCP and IFF; some plots showing the performance of some of these new WPs can be found here. We will remind them as soon as this MR gets accepted and the updated prescriptions announced.

A sentence starting with "I've not been following the IFF" and then saying something like "couldn't see a comparison" triggers an obvious answer... :-) Anyway, @maklein and @oducu will surely be able to provide more details if need be, and to update the IFF TWiki with the missing info, where necessary.

Thanks a lot for your prompt reply, really, and for having followed up with the failing tests: it's really crucial to get these unit tests updated as soon as possible, such that this MR can be accepted and the new MCP recommendations advertised.

Cheers, Stefano

Hi all,

I am testing what Ian suggested to do and I get this error for the most recent TOPQ1 derivations:

terminate called after throwing an instance of 'SG::ExcBadAuxVar'
  what():  SG::ExcBadAuxVar: Attempt to retrieve nonexistent aux data item `::AnalysisTop_Isol_FCTight_FixedRad' (1681).
Aborted (core dumped)

This is also true for FCTightTrackOnly_FixedRad. So it seems we are missing the needed variables in our derivations. Trying to investigate if we can run on at least one WP just to prevent the crash.

Cheers, Tomas

Hi Tomas

Okay, that error is not as bad as I feared. It means an update is also required here: https://gitlab.cern.ch/atlas/athena/blob/21.2/PhysicsAnalysis/TopPhys/xAOD/TopCPTools/Root/TopIsolationCPTools.cxx to set up the correct WP tools, and an update here https://gitlab.cern.ch/atlas/athena/blob/21.2/PhysicsAnalysis/TopPhys/xAOD/TopSystematicObjectMaker/Root/MuonObjectCollectionMaker.cxx (and in the header) to retrieve the tools and then setup the appropriate decorations.

Ian

Hi Stefano

Thanks for the links, I did find a few plots, but I didn't see a comparison against Gradient so was not sure what an appropriate replacement is, and indeed the IFF page has multiple points about isolation and about what is experimental and what is/ is becoming the nominal.

I've not been following because its not strictly my responsibility anymore ;-) but so long as any needed variables were added to the smart slimming lists a few months ago so that most analyses will have them in their MC and data derivations by now, it should not be too difficult to add the updated WPs. Once Tomas updates the code, it should help reveal if the test file needs to be updated in cvmfs.

Cheers, Ian

Alright, fair enough. Thanks again!

mentioned in merge request !20971 (merged)

Hi all,

I prepared a MRR that should fix all the problems, but since this is a rather important change we will need to review it more carefully. Also we need to update the input files, otherwise the CI will not pass anyway.

Cheers, Tomas

canceled the automatic merge

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

marked as a Work In Progress

HI all,

the changes in AnalysisTop have just been merged (!20971 (merged)). So AT should not cause crashes anymore.

Cheers, Tomas

unmarked as a Work In Progress

Tried to remove the WIP to see how the tests do!

Jenkins please retry a build

This merge request affects 1 package:

• PhysicsAnalysis/MuonID/MuonIDAnalysis/MuonEfficiencyCorrections

Adding @morgens ,@nkoehler ,@jojungge as watchers

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

CI Result FAILURE

	AnalysisBase	AnalysisTop	AthAnalysis	AthDerivation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-20869-2019-02-12-01-24
AnalysisBase: number of compilation errors 0, warnings 0
AnalysisTop: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
AthDerivation: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST 33660]

added 1 commit

• 1d5734a4 - Revert "Refined the file return logic"

Compare with previous version

added 1 commit

• 03dcd159 - Revert "Isolation scale-factors between 3&4 GeV are taken from the low-pt sclae-factor map"

Compare with previous version

This merge request affects 1 package:

• PhysicsAnalysis/MuonID/MuonIDAnalysis/MuonEfficiencyCorrections

Adding @morgens ,@nkoehler ,@jojungge as watchers

This merge request affects 1 package:

• PhysicsAnalysis/MuonID/MuonIDAnalysis/MuonEfficiencyCorrections

Adding @morgens ,@nkoehler ,@jojungge as watchers

CI Result FAILURE

	AnalysisBase	AnalysisTop	AthAnalysis	AthDerivation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-20869-2019-02-12-13-48
AnalysisBase: number of compilation errors 0, warnings 9
AnalysisTop: number of compilation errors 0, warnings 9
AthAnalysis: number of compilation errors 0, warnings 9
AthDerivation: number of compilation errors 0, warnings 11
For experts only: Jenkins output [CI-MERGE-REQUEST 33686]

Hi all, @tdado, the test is still failing, after the following warning:

top::MuonScaleFactorCa...WARNING WE ARE MISSING THE FOLLOWING SYSTEMATICS: top::MuonScaleFactorCa...WARNING MUON_EFF_ISO_STAT_LOWPT top::MuonScaleFactorCa...WARNING MUON_EFF_ISO_SYS_LOWPT

@jojungge has then implemented some modifications to the package to make the systematics treatment more uniform, and we're updating the calibration area once again with some small refinements in the same direction and some cleanup (we took advantage of the extra week). You might have anyway to check the AnalysisTop tests and implement a fix to make the warning/failure above disappear.

Cheers, Stefano

Hi @szambito,

the question is we can update the code outside of this MR? We do not see this warnings when running AT with the changes that we have made recently. So it seems the warnings appear only after the changes in this MR?

Or in other words, can we update the AT code in such a way that it will work with both old (without this MR) and new (changes from this MR) code? If yes, can you point us to what exactly needs to be changed, if not we will need to do the changes in this MR directly.

Cheers, Tomas

Hi @tdado,

Apologies if I'm missing something, but I don't think you'll need to be backwards compatible here. MCP is changing things for muons, and as a client of the MCP tools AT needs to follow. I don't think you need to stay compatible with outdated MCP ways.

Happy to stand corrected or learn I misunderstood something here, of course! :)

Best, Christian

Hi Christian,

Generally I wold agree with you but I will let conveners (mainly @mvanadia) to decide about the backwards compatibility.

Aside from this issue, if this MR introduces and interface breaking change, we cannot fix this in a separate MR in AT, but the changes need to be implemented in this MR directly, otherwise the separate MR in AT would fail as it uses the old MCP tools in the CI tests. So I guess the best approach is to provide a patch for AT packages that someone will need to apply to this MR so everything works?

Cheers, Tomas

Hi @tdado,

sorry, but what do you mean with interface breaking change? Just the working point names were updated (old non-supported working points were removed).

Best, Nico

Hi Nico,

well I am not sure I understand the warnings then. We updated the isolation WP in the recent MR !20971 (merged). We used a supported WP and everything worked without problems (CI passed). But it seems this MR changes something that causes the warnings.

So even when the WP names are changed this is an interface breaking change as our code needs updates to work. But we cannot change the names in a separate MR as this will not work with the old code, or am i wrong about this?

Cheers, Tomas

Hi @tdado,

yes, @fsforza is going to replicate a new folder to the calibration area ~now since they made another update to the isolation scale factor files (no worries, no changes in the handling of the tool). Thus, currently, noone has to take action. We just wait until the folder is replicated, then change the calibration area property to trigger the CI again, and everything should be fine.

Best, Nico

added 1 commit

• 20012d7a - Update the calibration release

Compare with previous version

This merge request affects 1 package:

• PhysicsAnalysis/MuonID/MuonIDAnalysis/MuonEfficiencyCorrections

Adding @morgens ,@nkoehler ,@jojungge as watchers

mentioned in merge request !21067 (merged)

CI Result FAILURE

	AnalysisBase	AnalysisTop	AthAnalysis	AthDerivation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-20869-2019-02-12-15-49
AnalysisBase: number of compilation errors 0, warnings 0
AnalysisTop: number of compilation errors 0, warnings 1
AthAnalysis: number of compilation errors 0, warnings 0
AthDerivation: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST 33692]

Hi all,

the failure means that the file has not yet been propagated to the calibration area, correct?

Cheers, Tomas

Yes, the files are already on /eos/atlas/atlascerngroupdisk/asg-calib/MuonEfficiencyCorrections/190212_Winter_r21/ but not yet replicated to /cvmfs/atlas.cern.ch/repo/sw/database/GroupData/MuonEfficiencyCorrections/. So, we have to wait until it's there and restart the CI then.

added 2 commits

• 791c2370 - No complains from the SF tool with the new iso files
• 98c17378 - Merge branch 'MCP_WinterRecom' of ssh://gitlab.cern.ch:7999/jojungge/athena into MCP_WinterRecom

Compare with previous version

Jenkins please retry a build

hi all, all the files should be now in place (hopefully...), do you know if the CI test picks them up also from the asg eos area?

Hi @tdado,

I see what you mean above - thanks for clarifying!

Best, Christian

@fsforza, I'm quite sure they will only ever be found when synched to cvmfs, and having them in eos isn't enough.

CI Result SUCCESS

	AnalysisBase	AnalysisTop	AthAnalysis	AthDerivation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-20869-2019-02-12-16-44
AnalysisBase: number of compilation errors 0, warnings 9
AnalysisTop: number of compilation errors 0, warnings 10
AthAnalysis: number of compilation errors 0, warnings 10
AthDerivation: number of compilation errors 0, warnings 54
For experts only: Jenkins output [CI-MERGE-REQUEST 33687]

hi,

almost incredible, but it finished successfully!

If possible, let's merge it asap before that something else breaks :)

Thanks everybody for the hard work and dedication!

hi @fsforza @jojungge @nkoehler @szambito ,

I just had a look the the files in the eos area. I see "Iso_GradientIso_Z.root" but no corresponding "Iso_LowPt_GradientIso_Z.root" file. Does this mean that if we run AnalysisTop with the muon gradient isolation we still get a crash (as was happening in the CI tests of this MR some days ago)?

Because, as I was discussing yesterday with Federico and Stefano, we have problems for that in AT because, while we implemented already the new iso WPs, we have still tools to do the fake estimation only for gradient. We will implement soon the tools for the recommended WPs, but if it's possible to still be able to run with Gradient isolation for a bit of time that would be great. Hopefully in a couple of weeks we'll have the new fake estimation tools in AT.

Cheers, Marco

hi @mvanadia ,

the files with "LowPt" in the name should be only for specific analyses which would like to use iso-WPs in combination with the LowPt muonSelection WP in order to get a slightly smaller syst at ~3 GeV. For the regular users we suggest not to change anything. Hopefully the AT CI will not crash

hi @cohm , @krumnack , @pberta ,

as the CI is happy and the last changes were only the update of the calibArea, would it still be possible to merge this today?

It would be great to be sure that all is included before @maklein talk that is tomorrow morning.

Thanks a lot!

Cheers, Federico

Can someone confirm that this doesn't affect derivations?

Never mind, this is the scale factor...

merged

mentioned in commit 52a9a857

added sweep:ignore label