Fix of missing Forward tracks at negative eta in SiSpacePointsSeedMaker_ATLxk.cxx

This adds back in an absolute check on the dz/dr distance in seeding, which was removed in a previous MR: !31567 (merged)

Should fix issues reported in where forward tracks on the negative side disappeared: https://its.cern.ch/jira/browse/ATLIDTRKCP-250

FYI: @sroe, @goblirsc, @nkoehler

This merge request affects 1 package:

• InnerDetector/InDetRecTools/SiSpacePointsSeedTool_xk

Adding @goetz ,@gavrilen ,@sroe ,@amorley as watchers

added InnerDetector master review-pending-level-1 labels

added 1 commit

• 2664fcc2 - change fabs to std::abs

Compare with previous version

This merge request affects 1 package:

• InnerDetector/InDetRecTools/SiSpacePointsSeedTool_xk

Adding @goetz ,@gavrilen ,@sroe ,@amorley as watchers

CI Result SUCCESS (hash 3c3f3d18)

	Athena	AthSimulation	AnalysisBase	AthGeneration
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 15028]

CI Result SUCCESS (hash 2664fcc2)

	Athena	AthSimulation	AnalysisBase	AthGeneration
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 15029]

Hi @stonjek,

since you are L1 today, could this be reviewed? It is a needed fix for the reconstruction of SAF muons.

Thanks, Nico

From what I can see you define the const variable aTz in two nested for loops. In some cases the two nested for loops can be left with a goto statement. Some lines later you define aTz again, this time non-const.

I am afraid that this might lead to namespace conflicts and/or memory leaks. The concept of const and the goto statement in C++ is inherited from C.

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

Hi @stonjek, The idea of this MR is to restore the state before !31567 (merged) regarding this variable, and is purely a bugfix. So our suggestion would be to keep things as they are for this MR, and to allow the experts to clean up the logic in a follow-up MR. This is the focus of a task in jira, ATLIDTRKCP-263.

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

Since there are several variables with the same potential conflict at aTz I assume for now that the compiler handles this correctly.

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

I would think what is done here is OK on the goto = scope but prb can be written a bit cleaner

• https://en.cppreference.com/w/cpp/language/goto

• https://en.cppreference.com/w/cpp/language/statements#Compound_statements

• https://en.cppreference.com/w/cpp/language/scope

It jumps out of a scope (for -- loop ) but not inside the other from what I see ... , i.e jumps out before the next for-loop scope .

But from a quick read prb break could be used , this part reads like a FORTRAN 77 kind of goto to exit the loop (no break or EXIT ) , the goto labels are exactly after the loops and there is not back tranfer from what I can see either ... I would not suprised if there is some FORTRAN heritage given what this actually does ;)

merged

mentioned in commit c967a68b

added sweep:ignore label

Hi Nora Why do you think that change from fabs to std::abs can change someting

Hi Igor, its rather the change from no absolute to using it

	- if (Tz < dzdrmin or Tz > dzdrmax) continue;
        + float aTz = std::abs(Tz);
	+ if (aTz < dzdrmin or aTz > dzdrmax) continue;

Doing private tests, I see that this recovers the missing forwardTrackParticles, but logically I can't see it makes a difference unless dzdrmin/max changes