TRD Coordinate Transformations by jbarrella · Pull Request #5553 · AliceO2Group/AliceO2

jbarrella · 2021-02-25T09:18:18Z

Adding two functions written by @tdietel to perform the inverse transformation from spatial to pad-timebin coordinates.

TrackletTransformer::Local2RowColTime - Takes HCID and 3D space-point and returns a point in pad-timebin coordinates.

TrackletTransformer::Hit2RowColTime - Takes an o2::trd:Hit and calls TrackletTransformer::Local2RowColTime against its coordinates.

tdietel

I am not sure to which degree I trust my code... The overall coordinate transformations should be ok, but datails like correct handling of tilted pads or Lorentz angle might not be correct.

I think we can merge it if there is benefit, but it would probably be useful to add some tests and check this code before it gets forgotten. @jbarrella, maybe you can contact @bazinski about the technicalities - CMake seems to have nice support for unit tests.

I think at the very least we should check for consistency, i.e. that we get the same (or conistent results) if we apply spatial->pad and pad->spatial coordinates after each other.

martenole

Not really sure what we use these functions for? And if we ask for row/pad/timebin shouldn't we return integer values? What coordinate system are the o2::trd::Hit objects given in actually?
I did not go through the actual calculation in detail..

martenole · 2021-02-25T09:47:25Z

mGeo->getPadPlane(chamberID) would be easier to read

I was actually able to remove this line entirely and instead use the mPadPlane member variable that is updated with TrackletTransformer::loadPadPlane(int hcid).

jbarrella · 2021-03-09T13:36:09Z

@tdietel Finally had a chance to look at this again. Was doing more testing and discovered that padrows 0 and 15 (for HCID = 776) return padrow positions of 0.6528 and 15.3472 respectively after converting to spatial and then back to rct coords with the new function. I don't really understand this. It looks like the positions are being reported at the center of the padrow in units of inner padrow lengths? But then the center of padrow 1 is not 1.5, but something like 1.35. Also, if this were the case, I would have expected the center of padrow 0 to be at ~ 0.35 like padrow 15 with its center at ~ 15.35. Not sure if I'm missing something here? Also not certain about this 0.15 difference over half a padrow. Would expect it to be a bit smaller.

tdietel · 2021-03-09T15:08:58Z

@tdietel Finally had a chance to look at this again. Was doing more testing and discovered that padrows 0 and 15 (for HCID = 776) return padrow positions of 0.6528 and 15.3472 respectively after converting to spatial and then back to rct coords with the new function. I don't really understand this. It looks like the positions are being reported at the center of the padrow in units of inner padrow lengths? But then the center of padrow 1 is not 1.5, but something like 1.35. Also, if this were the case, I would have expected the center of padrow 0 to be at ~ 0.35 like padrow 15 with its center at ~ 15.35. Not sure if I'm missing something here? Also not certain about this 0.15 difference over half a padrow. Would expect it to be a bit smaller.

That's a tricky one - we are testing several things at once, and we are not really sure if it's a bug in one of the components, or a misunderstanding of what is happening in between. Maybe we need to test all of the components separately. As a first step, it would be useful to record not only the initial and final row/col/tb coordinates, but also the z position. It should be quite simple to translate from z to the expected result.

And just to confirm: HCID=776 corresponds to SM 12, L4S4, i.e. a L4C1 chamber with inner/outer pad length of 90/75 mm. Is that correct?

tdietel · 2021-03-09T15:11:39Z

One thing I do not understand is how and when a new pad plane is loaded. It should be loaded whenever the HCID changes, but there are many functions where the HCID is passed as an argument, but it is not checked if the loaded pad plane matches the requested HCID.

jbarrella · 2021-03-09T15:58:41Z

One thing I do not understand is how and when a new pad plane is loaded. It should be loaded whenever the HCID changes, but there are many functions where the HCID is passed as an argument, but it is not checked if the loaded pad plane matches the requested HCID.

Good point. In TrackletTransformer, the only function that uses mPadPlane and takes the HCID as an argument is TrackletTransformer::calculateY. Perhaps we can add a check in there to ensure that HCID matches the loaded pad plane. It doesn't look like there is a way to get the detector ID of the pad plane directly so I guess we would have to introduce another member variable loadedDetectorID or something to keep track of it.

jbarrella · 2021-03-09T16:02:21Z

@tdietel Finally had a chance to look at this again. Was doing more testing and discovered that padrows 0 and 15 (for HCID = 776) return padrow positions of 0.6528 and 15.3472 respectively after converting to spatial and then back to rct coords with the new function. I don't really understand this. It looks like the positions are being reported at the center of the padrow in units of inner padrow lengths? But then the center of padrow 1 is not 1.5, but something like 1.35. Also, if this were the case, I would have expected the center of padrow 0 to be at ~ 0.35 like padrow 15 with its center at ~ 15.35. Not sure if I'm missing something here? Also not certain about this 0.15 difference over half a padrow. Would expect it to be a bit smaller.

That's a tricky one - we are testing several things at once, and we are not really sure if it's a bug in one of the components, or a misunderstanding of what is happening in between. Maybe we need to test all of the components separately. As a first step, it would be useful to record not only the initial and final row/col/tb coordinates, but also the z position. It should be quite simple to translate from z to the expected result.

And just to confirm: HCID=776 corresponds to SM 12, L4S4, i.e. a L4C1 chamber with inner/outer pad length of 90/75 mm. Is that correct?

That is correct. Okay, I'll look more closely at the spatial coordinates as well. I can look up the dimensions of the pad plane and we can calculate what the correct values should be. Was it your intention to report these values in units of inner padrow lengths though? So you were not expecting to see 0.5 and 15.5 or were you? I guess standardizing the units within each chamber would make sense.

martenole

Some comments inline

jbarrella · 2021-04-26T11:56:02Z

@martenole @bazinski Hello! Hoping to merge this soon. Two main points:

Firstly, after much discussion and doodling (https://miro.com/app/board/o9J_lKgybMc=/) we feel that we understand the pad tilting enough to confirm the current solution. So I have not changed the pad tilting direction which I was initially concerned about. However, I did overload PadPlane::getTiltOffset() since the old function did not consider the length of the outer pad rows. Instead, it assumed that the pads were tilted about a point that is a distance of 1/2 of an inner pad length into the pad i.e. not the center. I wasn't sure of the best way to test whether this was correct or not. What I ended up doing was measuring a few distances in fig. B.11 of David Emschermann's thesis. This is a figure of the pad plane. I found that the distances I measured agreed with a pad tilt about the center and not about a distance of 1/2 of an inner pad length into the pad. So I changed that.
I overloaded the function instead of replacing it because I noticed that it was used by Digitizer.cxx. I can try to update the code in there if we're happy with the new PadPlane::getTiltOffset() function.

Let me know what you think. Thanks!

jbarrella · 2021-04-29T11:59:44Z

After some consideration and a chat with @bazinski, I've gone ahead and changed the calls in Digitizer.cxx to call the new PadPlane::getTiltOffset() function that takes into account the difference in length of inner and outer pads.

@jolopezl please take a look at the changes in Digitizer.cxx and make sure they look reasonable. The new function needs an additional argument which is the pad row number. In your code, it looked like this was rowE.

jolopezl

@jolopezl please take a look at the changes in Digitizer.cxx and make sure they look reasonable. The new function needs an additional argument which is the pad row number. In your code, it looked like this was rowE.

The change looks reasonable. In the digitizer, rowE is the row position after diffusion so that the change looks good.

I left some comments that you could take care of. This is a good chance to improve what's in PadPlane.

jbarrella · 2021-05-05T08:18:11Z

Hi @davidrohr, can we go ahead and merge this?

jbarrella requested review from bazinski, jolopezl, martenole and tdietel as code owners February 25, 2021 09:18

tdietel reviewed Feb 25, 2021

View reviewed changes

martenole reviewed Feb 25, 2021

View reviewed changes

jbarrella force-pushed the tracklet-transformer-patch-v3 branch from 7f34cea to bc01709 Compare March 8, 2021 13:24

rpezzi mentioned this pull request Mar 10, 2021

Adding FT3 detector for ALICE3 EndCaps simulation #5526

Merged

jbarrella changed the title ~~TRD TrackletTransformer patch~~ [WIP] TRD TrackletTransformer patch Mar 10, 2021

jbarrella force-pushed the tracklet-transformer-patch-v3 branch 3 times, most recently from 26f1f6b to 8a3a3ba Compare March 25, 2021 13:37

jbarrella changed the title ~~[WIP] TRD TrackletTransformer patch~~ [WIP] TRD Coordinate Transformations Mar 25, 2021

jbarrella force-pushed the tracklet-transformer-patch-v3 branch from 8a3a3ba to 41c8b48 Compare March 28, 2021 20:15

martenole reviewed Mar 29, 2021

View reviewed changes

jbarrella force-pushed the tracklet-transformer-patch-v3 branch from 41c8b48 to 12f590e Compare April 8, 2021 11:14

jbarrella force-pushed the tracklet-transformer-patch-v3 branch from 12f590e to 2df13ba Compare April 15, 2021 19:36

jbarrella force-pushed the tracklet-transformer-patch-v3 branch from 2df13ba to 53698b5 Compare April 26, 2021 11:42

jbarrella force-pushed the tracklet-transformer-patch-v3 branch from 53698b5 to 1e368fb Compare April 29, 2021 11:56

jbarrella changed the title ~~[WIP] TRD Coordinate Transformations~~ TRD Coordinate Transformations Apr 29, 2021

jbarrella force-pushed the tracklet-transformer-patch-v3 branch from 1e368fb to 9a96936 Compare April 29, 2021 16:58

jolopezl suggested changes May 1, 2021

View reviewed changes

martenole reviewed May 4, 2021

View reviewed changes

Comment thread Detectors/TRD/base/include/TRDBase/PadPlane.h Outdated

jbarrella force-pushed the tracklet-transformer-patch-v3 branch 3 times, most recently from b7f6c71 to 9af595e Compare May 4, 2021 11:38

jolopezl previously approved these changes May 4, 2021

View reviewed changes

tdietel previously approved these changes May 4, 2021

View reviewed changes

martenole reviewed May 4, 2021

View reviewed changes

Comment thread Detectors/TRD/base/src/PadPlane.cxx Outdated

new coordinate transformation functions in PadPlane with CMake tests

736c321

jbarrella dismissed stale reviews from tdietel and jolopezl via 736c321 May 4, 2021 12:13

jbarrella force-pushed the tracklet-transformer-patch-v3 branch from 9af595e to 736c321 Compare May 4, 2021 12:13

martenole approved these changes May 4, 2021

View reviewed changes

jolopezl approved these changes May 4, 2021

View reviewed changes

davidrohr merged commit 3ffab76 into AliceO2Group:dev May 5, 2021

Conversation

jbarrella commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdietel left a comment

Choose a reason for hiding this comment

Uh oh!

martenole left a comment

Choose a reason for hiding this comment

Uh oh!

martenole Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

jbarrella Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbarrella commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tdietel commented Mar 9, 2021

Uh oh!

tdietel commented Mar 9, 2021

Uh oh!

jbarrella commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbarrella commented Mar 9, 2021

Uh oh!

martenole left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbarrella commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbarrella commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jolopezl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jbarrella commented May 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

jbarrella commented Feb 25, 2021 •

edited

Loading

jbarrella commented Mar 9, 2021 •

edited

Loading

jbarrella commented Mar 9, 2021 •

edited

Loading

jbarrella commented Apr 26, 2021 •

edited

Loading

jbarrella commented Apr 29, 2021 •

edited

Loading

jolopezl left a comment •

edited

Loading