TRD add additional helper functions by pestratm · Pull Request #11644 · AliceO2Group/AliceO2

pestratm · 2023-07-10T12:44:45Z

Added some additional helper functions for TRD that @tdietel provided in his macros
@martenole @pachmay

martenole

Hi @pestratm thanks, the update to the HelperMethods is fine with me. For the tracklet64 I think the existing functions should be OK. Can you see if they are sufficient for the macros you want to add to the TRD repo?
And please not in general functions in O2 need to start with lower case letter

martenole · 2023-07-10T13:03:12Z

DataFormats/Detectors/TRD/src/Tracklet64.cxx

+  return y / padWidth + 71.0;
+}
+
+GPUd() float Tracklet64::Slope() const


could for this not getUncalibratedDy(1) be used? And I think one needs to take into account the pad width for the slope, no?

If we take into account the pad width, then we can use getUncalibratedDy(1). Tbh, I'm not sure about Tom's reasoning not to take it into account.

The slope is given in units of pad/timebin, while dy is a distance in cm.
For my raw data display, I wanted everything in pads and timebins, hence the extra function.
I find code with getSlope() more readable than writing the below calculation, but that might be personal preference.

martenole · 2023-07-10T13:04:41Z

DataFormats/Detectors/TRD/src/Tracklet64.cxx

+  return float((mcmCol + 1) * NCOLMCM) + 2.0 - padMCM;
+}
+
+GPUd() float Tracklet64::UncalibratedPad() const


isn't this a duplicate of getUncalibratedY()?

UncalibratedPad() gives the uncalibrated pad number corresponding to the tracklet position, while getUncalibratedY() gives the uncalibrated y-coordinate. That's why for UncalibratedPad(), we divide by the pad width, isn't it?

but the uncalibrated pad number should really be given by getPadCol(). I just want to avoid any confusion and that we use consistently the functions we have in all the places. Otherwise it get's quite messy. So maybe you could use the functions in your PR to the TRD repo which are already available and drop the ones which are added to Tracklet64? The additional function in HelperMethods is fine of course

It looks like this essentially does the same thing as PadPosition() discussed above, but in a different way (i.e. calculating the pad from the y coordinate, rather than from MCM and position within the MCM).
I don't remember exactly, but I might have compared the two to make sure I get it right.

martenole · 2023-07-10T13:05:42Z

DataFormats/Detectors/TRD/src/Tracklet64.cxx

+  return 12.0 - (getPositionBinSigned() * GRANULARITYTRKLPOS);
+}
+
+GPUd() float Tracklet64::PadPosition() const


isn't this a duplicate of getPadCol()?

isn't this a duplicate of getPadCol()?

getPadCol() returns an int, but the position of a tracklet is known better than a pad. Therefore I think it makes sense to provide a function that gives the tracklet position with float precision.
I suspect changing getPadCol() to return a float might break some code, hence I introduced another helper function. I am thinking about calling it getPadColF() (or getPadColFloat, suggestions welcome).

martenole · 2023-07-10T13:07:20Z

DataFormats/Detectors/TRD/src/Tracklet64.cxx


+GPUd() float Tracklet64::PadPositionMCM() const
+{
+  return 12.0 - (getPositionBinSigned() * GRANULARITYTRKLPOS);


getPositionBinSigned() * GRANULARITYTRKLPOS should give the local pad number relative to the MCM center. Why are you subtracting 12 here? Shouldn't it be 10.5? But in any case I am not sure if these functions are needed

From my memory, the reason for 12 is that it matches the digits in the raw data display. At the time, I did not try to figure out why this works.

in UncalibratedPad() there is also a +2 which I don't understand. This effectively turns the 12 into a 10, so closer to the expected 10.5

alibuild · 2023-07-12T18:24:31Z

Error while checking build/O2/fullCI for cfb7136 at 2023-07-12 20:24:

No log files found

Full log here.

martenole · 2023-07-14T11:59:25Z

Ok, I'd suggest we add

getPositionInPadCol() to return the TRD pad column number as a float
getSlopeInPadsPerTB() to return the slope as float in units of pads per time bin
These functions should then be used internally by getPadCol(), getUncalibratedY() and getUncalibratedDy().

But we still have to understand the offset. In the getPadCol() I currently set an offset of 8 pads, because this is what I see when matching digits to tracklets on tracks and simply looking for the maximum ADC sum inside the digits. On average this should give me the correct offset for the conversion, no? I probably need to look closer again.

@pestratm do you want to give it a try to implement the above 2 functions to Tracklet64 or should I do it?

pestratm · 2023-07-14T12:10:21Z

Ok, I'd suggest we add
* `getPositionInPadCol()` to return the TRD pad column number as a float

* `getSlopeInPadsPerTB()` to return the slope as float in units of pads per time bin
  These functions should then be used internally by `getPadCol()`, `getUncalibratedY()` and `getUncalibratedDy()`.
But we still have to understand the offset. In the getPadCol() I currently set an offset of 8 pads, because this is what I see when matching digits to tracklets on tracks and simply looking for the maximum ADC sum inside the digits. On average this should give me the correct offset for the conversion, no? I probably need to look closer again.

@pestratm do you want to give it a try to implement the above 2 functions to Tracklet64 or should I do it?

You mean keeping PadPosition() as getPositionInPadCol(), and Slope() as getSlopeInPadsPerTB()?

github-actions · 2023-08-14T00:51:36Z

This PR did not have any update in the last 30 days. Is it still needed? Unless further action in will be closed in 5 days.

TRD add additional helper functions

cfb7136

pestratm requested review from bazinski, f3sch, martenole, pachmay and tdietel as code owners July 10, 2023 12:44

martenole suggested changes Jul 10, 2023

View reviewed changes

github-actions bot added the stale label Aug 14, 2023

github-actions bot closed this Aug 20, 2023

Conversation

pestratm commented Jul 10, 2023

Uh oh!

martenole left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alibuild commented Jul 12, 2023

Uh oh!

martenole commented Jul 14, 2023

Uh oh!

pestratm commented Jul 14, 2023

Uh oh!

github-actions bot commented Aug 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants