Bugfix to fix a problem in TRT/dEdx

According to the following JIRA ticket https://its.cern.ch/jira/browse/ATLASRECTS-4391, this MR should fix the floating point exceptions for TRT_ToT_dEdx by using a deprecated option in the tools. Code changes were made in collaboration with @cgrefe and @korona.

added InnerDetector bugfix + 1 deleted label

This merge request affects 1 package:

• InnerDetector/InDetRecTools/TRT_ToT_Tools

added 21.0 review-pending-level-1 labels

CI Result FAILURE

	Athena	AthDataQuality	AthSimulation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-23728-2019-05-27-17-16
Athena: number of compilation errors 1, warnings 0
AthDataQuality: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST 39020]

added 1 commit

• b43fd7e2 - Fixed problem with Athena warning

Compare with previous version

This merge request affects 1 package:

• InnerDetector/InDetRecTools/TRT_ToT_Tools

CI Result FAILURE

	Athena	AthDataQuality	AthSimulation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-23728-2019-05-27-22-22
Athena: number of compilation errors 0, warnings 0
AthDataQuality: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST 39042]

This changes the q-test output is that expected?

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

Hey @tadej, what exactly is the q-test? Cheers, Philipp

It's a standard test ensuring that the output of the reconstruction does not change. I think the q stands for quality

Please have a look here: http://cern.ch/atlas-computing/links/distDirectory/ci/CIWebArea/nicos_web_areaMRCIbuilds64BS6G62AthenaOpt/NICOS_TestLog_MR-23728-2019-05-27-22-22/ReleaseTests___qTestsTier0_required-test__qTestsTier0_required-test__m.html

Hi Tadej,

Yes, we expected some changes in the TRT_ToT_dEdx output. The main goal of the changes is to block some external calls of the TRT_ToT_dEdx::dEdx() and TRT_ToT_dEdx::usedHits() functions with non-recommended or obsolete parameters. As I understood the problem is that our new tag brakes the frozen tier0 policy. What would you recommend to overcome this situation?

Best, Natalia

Hi @korona, I was under the impression that this change only affects an outdated package that is not part of the standard dEdx calculation. If it does affect the output of the standard dEdx calculation then we have to protect the change with a switch such that the default output is unchanged but it could be enabled via job options. For master this switch can then be removed again.

Hi Christian,

The difference in the standard dEdx calculation is due to the change of the default ToT calculation algorithm (LargerIsland algorithm instead of HighOccupancySmart). I agree with your suggestion to protect the changes with a switch enabled via job options. But would you please give me a hint how to create such a switch.

Best, Natalia

Hi Natalia,

just add another parameter to the class similar to TRT_dEdx_divideByL and protect all changes with an if-else structure depending on the new parameter. Then set the default such that the old behaviour is used.

Cheers, Christian

@Christian, thank you for the explanation.

@Philipp, to apply the required switch we need to insert several lines in the TRT_ToT_dEdx.h and TRT_ToT_dEdx.cxx files. Would you like me to prepare a new version? If yes, what would be the best way to do so? (prepare a list of changes?)

Best, Natalia

Hey @korona,

yes, if you want you can just send me the changes I need to insert and then I will add them to the MR. I'm a bit confused about your statement "difference in the standard dEdx calculation is due to the change of the default ToT calculation algorithm". Why does our changes affect the ToT calculation algorithm?

Cheers, Philipp

Hi Philipp,

If I understand your question correctly, since dEdx is calculated (roughly speaking) as the average ToT value on a track, we see the difference when switching the default ToT algorithm from HighOccupancySmart to LargerIsland.

Best, Natalia

Hi Philipp, I prepared a list of changes to insert into the last 21.0 commit as a text file (attached). Please don't hesitate to contact me if you have questions/commentsrel21-changes.rtf.

Best, Natalia

Hi Philipp, I added one small change to the point (8) of the list related to the masking 4 last bits in the ToTlargerIsland. It was not a good idea to use common variable name "m_word" as an auxiliary variable inside getToTLargerIsland(). I changed it to "BitPattern0". I highlighted this small change with yellow in the updated list of changes (attached) rel21-changes.rtf.

Best, Natalia

marked as a Work In Progress

added 1 commit

• 7ee9489e - Protected all changes by inserting a switch. Changes will only be applied if...

Compare with previous version

added 1 commit

• 24ab8d79 - Small changes to make the code more efficient

Compare with previous version

unmarked as a Work In Progress

This merge request affects 1 package:

• InnerDetector/InDetRecTools/TRT_ToT_Tools

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

This merge request affects 1 package:

• InnerDetector/InDetRecTools/TRT_ToT_Tools

CI Result SUCCESS

	Athena	AthDataQuality	AthSimulation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-23728-2019-06-03-23-00
Athena: number of compilation errors 0, warnings 162
AthDataQuality: number of compilation errors 0, warnings 1
AthSimulation: number of compilation errors 0, warnings 5
For experts only: Jenkins output [CI-MERGE-REQUEST 39381]

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

Hi Philipp,

1. To comply with the L1 shifters request I would suggest to change the line https://gitlab.cern.ch/phkoenig/athena/blob/7ee9489e70d91e13001fd7d2ffc59f34748ff367/InnerDetector/InDetRecTools/TRT_ToT_Tools/TRT_ToT_Tools/TRT_ToT_dEdx.h#L65 to the following one:

bool m_applyBugfix; // If true - calculate dEdx using only recommended settings (DivideByL=true, useHThits=true, m_toolScenario=kAlgReweightTrunkOne)

2. Looking at the last commit https://gitlab.cern.ch/phkoenig/athena/blob/7ee9489e70d91e13001fd7d2ffc59f34748ff367/InnerDetector/InDetRecTools/TRT_ToT_Tools/TRT_ToT_Tools/TRT_ToT_dEdx.h I do not see new dEdx() and usedHits() classes with the new bool applyBugfix switch introduced. Probably we have some misunderstanding of the problem. From my point of view, we need to introduce a new dEdx and usedHits classes with the new bool applyBugfix. We have already adjusted all the default setting in the TRT_ToT_dEdx::Initialize() part. But Athena does not read it and calls directly dEdx() and usedHits() functions. To get correct TRTdEdx and TRTdEdxUsedHits variables in the xAoD Athena (TrkTrackSummaryTool) will need to call new dEdx() and usedHits() functions with additional parameter bool applyBugfix=true.

If you agree please add new dEdx class:

/** * @brief function to calculate sum ToT normalised to number of used hits * @param track pointer * @param bool variable to decide wheter ToT or ToT/L should be used * @param bool variable whether HT hits shoule be used * @param bool variable to apply only recommended dEdx settings * @return ToT / double dEdx(const Trk::Track, bool DivideByL, bool useHThits, bool corrected, bool applyBugfix);

and new usedHits class:

/** * @brief function to calculate number of used hits * @param track pointer * @param bool variable to decide wheter ToT or ToT/L should be used * @param bool variable whether HT hits should be used * @param bool variable to apply only recommended dEdx settings * @return nHits / double usedHits(const Trk::Track track, bool DivideByL, bool useHThits, bool applyBugfix);

Best, Natalia

Dear @korona,

I didn't care of this since I was on vacation. Concerning your points:

1. Yes, I'll do that.
2. I discussed this point with @cgrefe. We certainly don't want to have new dEdx() and usedHits() classes. The idea is to introduce a new global bool parameter (m_applyBugfix), which choses the old behaviour when set to false (default value) or applies all the bug fixes when set to true. And some point the TRT_ToT_dEdx has to be defined before you can call the functions dEdx() and usedHits(), so it will for sure call the Initialize() function.

Cheers, Philipp

added 1 commit

• 4c732fd2 - Extended description for bugfix and correct naming with suffix m_ for private data members

Compare with previous version

This merge request affects 1 package:

• InnerDetector/InDetRecTools/TRT_ToT_Tools

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

CI Result SUCCESS

	Athena	AthDataQuality	AthSimulation
externals
cmake
make
required tests
optional tests

Full details available at NICOS MR-23728-2019-06-19-11-58
Athena: number of compilation errors 0, warnings 0
AthDataQuality: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST 39612]

Are all the suggestions taken care of? The discussions need to be 'resolved' before this can be approved. Ruchi (L1)

resolved all discussions

All discussions are resolved now. Approving...

Lino (L1)

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

merged

mentioned in commit 1aa69838

added sweep:done label

added sweep:failed label

Sweep summary
failed:

• master

mentioned in merge request !24614 (merged)

mentioned in commit d9dd5a6d