remove overwriting of input parameters in TVSurface::CalcXingPointWith

[danieljeans]

BEGINRELEASENOTES

• don't ignore input params "eps" and "mode" of TVSurface::CalcXingPointWith
• this would address the (potential) issue of TVSurface: ignored parameters? #6
  ENDRELEASENOTES

[tmadlener]

Hi @danieljeans, I agree overwriting arguments seems a bit strange. A minor concern that I have here, is that the default value for eps is 1e-8:

KalTest/src/geomlib/TVSurface.h

Line 44 in 45a72f3

Double_t eps = 1.e-8) const;

Adapting that to the previously hardcoded value of 1e-5 would avoid changing things where no eps was passed. Could you change that as well?

For mode I am not entirely sure if there will be problems if removing the hardcoded value of 0. However, there is also not really something we can do about it. At least a quick iLCSoft wide search for TVSurface gave a few hits. I could try to flush them out with compiler warnings when I do the next HEAD build, unless you think this should not affect things to much in any case (also because there is a second overload without mode, that just forwards with mode set to 0).

[tmadlener]

Hi @danieljeans, I checked where this shows up throughout the stack. As you can see in the overview below there are a few cases, where the value of mode is not directly visible from the calling site, because it is either just passed on as an argument, or deduced from other variables. There are also a few cases, where it is explicitly set to something else than 0 and then there are a few cases, where it is indeed set to 0 explicitly or implicitly by calling the overload without mode. So overall, I am not entirely sure what the effects of this change would be in practice. Maybe you have a bit more intuition in this direction?

mode as variable (value not easily deducible)

• KalTest/src/geomlib/TCylinder.cxx

  Line 74 in 45a72f3

  if(!hel.IsInB() || !TBField::IsUsingUniformBfield()) return TVSurface::CalcXingPointWith(hel, xx, phi, mode, eps);

• KalTest/src/kaltracklib/TKalDetCradle.cxx

  Line 263 in 45a72f3

  if (static_cast<TVSurface *>(At(ito))->CalcXingPointWith(hel, xx, fid, mode, eps)) { // if we have a crossing point at this surface, note di specifies if we are moving forwards or backwards

• KalTest/src/kaltracklib/TKalDetCradle.cxx

  Line 404 in 45a72f3

  dynamic_cast<TVSurface *>(At(ito))->CalcXingPointWith(hel, xx, fid, mode);

• KalTest/src/kaltracklib/TKalDetCradle.cxx

  Line 414 in 45a72f3

  dynamic_cast<TVSurface *>(At(ito))->CalcXingPointWith(rk, rkxx, step, mode);

• https://github.com/iLCSoft/DDKalTest/blob/81e946b08c3d244c762714e707b640dae0b8152d/src/DDConeMeasLayer.cc#L134 (via TCutCone that inherits from TVSurface)

mode != 0 explicitly set at calling site

• https://github.com/iLCSoft/KalDet/blob/a667b1799f185e3ed201eaec5d40ce9e685d95e5/gen/EXEventGen.cxx#L117
• https://github.com/iLCSoft/MarlinTrk/blob/c53d868979ef6db26077746ce264633819ffcf4f/src/MarlinDDKalTest.cc#L400 (set in line 382, to either +1 or -1)

mode == 0 at calling site

• KalTest/src/kaltracklib/TKalDetCradle.cxx

  Line 220 in 45a72f3

  sfp->CalcXingPointWith(hel, xto, fito, 0, eps);

• https://github.com/iLCSoft/DDKalTest/blob/81e946b08c3d244c762714e707b640dae0b8152d/src/DDPlanarMeasLayer.cc#L272
• https://github.com/iLCSoft/KalDet/blob/a667b1799f185e3ed201eaec5d40ce9e685d95e5/ild/common/ILDConeMeasLayer.h#L75
• https://github.com/iLCSoft/KalDet/blob/a667b1799f185e3ed201eaec5d40ce9e685d95e5/ild/common/ILDRotatedTrapMeaslayer.h#L75

calling the "no mode" version (i.e. mode == 0 by definition)

• KalTest/src/kaltracklib/TKalTrackSite.cxx

  Line 109 in 45a72f3

  return ms.CalcXingPointWith(*hel,xx,phi,eps);

• KalTest/src/kaltracklib/TKalTrackSite.cxx

  Line 112 in 45a72f3

  return ms.CalcXingPointWith(*hel,xx,phi);

• https://github.com/iLCSoft/MarlinTrk/blob/c53d868979ef6db26077746ce264633819ffcf4f/src/MarlinDDKalTestTrack.cc#L485

[danieljeans]

changed default eps to 1e-5 which should preserve current behaviour.
Keisuke agrees this is a reasonable default value, when using mm for length units.
This eps is the tolerance on the squared distance to the surface, so 10-5 corresponds to a few microns, probably precise enough for a general case.

[danieljeans]

@tmadlener thanks for checking where "mode" is used.
To be honest I don't know this code so well.
Keisuke (original author) noticed this forcing of "mode=0", describing it as a bug.
He explained that mode=0 means we look for the intersection closest to the current position, while mode=+(-)1 looks for the next intersection forwards(backwards) along the track.
He is still looking into the effect of this "mode=0" overwriting, so I suggest we wait for his conclusion before merging this.

[danieljeans]

after some more checks, Keisuke recommends to remove the "mode=0" line.
It may be a good idea to remake some usual tracking efficiency plots (eg for ILC) as a final check before merging?
Who knows how to make them?

[tmadlener]

OK. In that case I would leave this out of the current patch release to let that move forward. We can then make another one once we have a solution for this.

It may be a good idea to remake some usual tracking efficiency plots (eg for ILC) as a final check before merging?
Who knows how to make them?

I agree. I am not sure if there is someone who routinely makes them at the moment. In principle the necessary scripts should all be somewhere. I will try to find out.