Bug fix in Segmentation's CylindricalGridPhiZ and PolarGridRPhi2: phi…

[ybedfer]

… range

is now set to [-pi,pi] instead of [0,2pi] if phi IDdescriptor field is signed.

BEGINRELEASENOTES

• Bug fix in CylindricalGridPhiZ and PolarGridRPhi2:
  • In the cellId member method, one now refrains from reducing the phi parameter modulo 2pi when its IDdescriptor is signed.
  • When the offset is null, this means that the phi range is now [-pi,pi], instead of the previously [0,2pi] which is nonsense for a signed quantity.

ENDRELEASENOTES

[andresailer]

What is the bug? And why is it nonsense?
I am very reluctant to change this behaviour!

[ybedfer]

What is the bug? And why is it nonsense? I am very reluctant to change this behaviour!

• The bug is: reducing the phi value modulo 2pi when it happens to be less than the offset. This, in any case.
• When the offset is null (which is the default), this means preventing phi from being negative. In my understanding, having a negative sign in the IDdescriptor is precisely meant to handle negative quantities. This is what I call "nonsense".
• With my fix, I am restricting the modulo(2pi) to cases where the parameter is not signed. Keeping the behaviour unchanged in these cases.
• The bug turned out to upset my strip digitization of the CylindricalGridPhiZ hits.
• I have encountered no problem w/ PolarGridRPhi2, since I am not using it. But I noticed that the very same modulo(2pi), nonsensical in my mind, was also applied in any case there. So I changed it too.

[andresailer]

Ok, thanks for the explanation!

The only case I see where PolarGridRPhi2 is used with signed phi is (does not mean others are in the wild):

https://github.com/key4hep/k4geo/blob/1d2721d13e8562dd8521c72cc6a81ce33ac072d7/FCalTB/compact/TestBeamSetup.xml#L16-L20

But that should be fine given the spanning of the sensor there?

[github-actions]

Test Results

   14 files     14 suites   6h 2m 25s ⏱️
  368 tests   368 ✅ 0 💤 0 ❌
2 531 runs  2 531 ✅ 0 💤 0 ❌

Results for commit aaee9cb.

[ybedfer]

But that should be fine given the spanning of the sensor there?

Yes, it should. They even write in comment phi bin must not be negative in this case.
My guess would be that they overlooked the meaning of the sign in the IDdescriptor. Their segmentation must work the same if they remove it.

I can add that there exists another (r,phi) segmentation class, viz. PolarGridRPhi (w/out the '2'). And there, there is no modulo(2pi) (may be a mistake then, since phi is still given by atan2, hence [-pi,pi], while w/ an unsigned parameter, you would expect mainly positive values, as far as I understand).

Last, in PolarGridRPhi2, this time, there was a modulo(2pi) in the position(const CellID& cID) method. Completely useless. Meaning, in my mind, that the whole thing was not investigated very deeply...

Giving a second thought: introducing such fix w/o pre-warning could be problematic. Not so much for CylindricalGridPhiZ, since I have introduced it few months ago and not much work must have been put to it, but for PolarGridRPhi2. What I can do is;

• Prepare a new class, let's say PolarGridRPhiA (I could not find any better word), w/ the fix.
• Declare the older one deprecated.

[andresailer]

Hi @ybedfer

There is another option for PolarGridRPhi2: just leave it as it was before.

Btw: the "they" you are referring to is mostly me :)

[andresailer]

Agreed that this is totally useless.

[ybedfer]

There is another option for PolarGridRPhi2: just leave it as it was before.

I have done as you suggests.
I did this by undoing the commits to my fork repository. And committing again the changes to CylindricalGridPhiZ alone.
As a consequence, the PullRequest is closed, or so it seems.
I am starting a new one....