TauAnalysisTools: change default truth jet container in tau truth matching

Hello,

This MR is changing the default truth jet container used for tau truth matching from AntiKt4TruthJets to AntiKt4TruthDressedWZJets, as the latter is much more widely used. It's a follow-up of !68328 (merged) . This will hopefully fix the AnalysisAlgorithmsConfig ctest failure. Not sure what the test was doing so far... it was also using the wrong container, but maybe the use of data handles creates a louder error.

Cheers, Bertrand

This merge request affects 1 package:

• PhysicsAnalysis/TauID/TauAnalysisTools

This merge request affects 1 file:

• PhysicsAnalysis/TauID/TauAnalysisTools/TauAnalysisTools/BuildTruthTaus.h

Adding @martindl as watcher

added Analysis Tau analysis-review-required main review-pending-level-1 labels

added urgent label

mentioned in merge request !69560 (merged)

CI Result SUCCESS (hash 325ed6f1)

	Athena	AnalysisBase	AthAnalysis
externals
cmake
make
tests

Full details available on this CI monitor view. Check the JIRA CI status board for known problems
Athena: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-EL9 6331] (remote access info)

Hi @wlampl , this MR should be ready for merging.

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

Looks good to me. Approved.

Cheers L1

merged

mentioned in commit a0181e78

For the next time: When you think your change affects a test outside of the package that you modify, please put full-unit-tests on the MR. Hopefully this fixed the issue, but I would not be able to tell easily...

To be clear: Making the original mistake, which made the test fail, was an honest mistake. No complaints about that. But during the fix we should really make sure that the CI would actually run all the relevant tests.

Hi @akraszna I agree with you but some times it is not so trivial to know what the relevant tests are. It would be good to have a centralize way to be able to perform tests even if other packages have been modified.

I know the LABELS property would allow for that, but when trying to move to that we had failures observed in the nightlies even if CI and local tests were perfectly fine.

The only solution is to have CI tests but if you rely on a reference text file (for instance) that is not that trivial and, as far as I know, there is no centrally provided macro for allowing this easily. That is what we experienced in ACTS (and I'm still trying to address).

There are tests, such as the trigger count changes test, that do what I'm asking but that is not really easy to implement from scratch for other uses and rely on ad hoc python files.

Let me explain why I got annoyed.

• Unit test failures, associated to the AnalysisAlgorithmConfig package appeared in CI jobs;
  • As I wrote earlier, this is par for the course. I don't blame anyone for introducing such an error into a high-level package, which uses a whole lot of other packages;
• Bertrand quickly identified that !68328 (merged) is probably what caused these failures. So far, so good.
• He opens this MR to fix the test failures introduced by !68328 (merged).
• But this MR, just like !68328 (merged), does not set up the CI to run the tests from AnalysisAlgorithmConfig. While for !68328 (merged) that was not a mistake, for this MR the unit tests of AnalysisAlgorithmConfig should've been activated in the CI.

To do the last part, the full-unit-tests flag should've been put on the MR. Then it would've been obvious from the CI's results that this change indeed fixes (or doesn't...) !68328 (merged).

But now we have to wait for the next CI job that will run the unit tests from AnalysisAlgorithmConfig, started not longer than 1 hour ago, to see if this MR did what it was supposed to do.

And yes, I got annoyed because I have a very long-running MR myself (!69721 (merged)), whose CI failed because of this mistake. But right now I'm still not 100% sure if the issue that my MR experienced is indeed fixed by now or not.

Right... point taken, I should have added the full unit test label here. I have run all the unit tests from the AnalysisAlgorithmConfig package locally, to confirm the previous error was gone, but obviously the results are not visible to everybody.

mentioned in merge request !69721 (merged)