Update position_from_cell calculation

[fredevb]

• Added half of pitch in position_from_cell calculation to obtain the center.
• Changed the name of min_center_x and min_center_y to min_corner_x and min_corner_y in the pixel data structure.

[asalzburger]

Hi @krasznaa - we encountered that in comparing cluster positions between ACTS and traccc - that showed a systematic shift of half a pixel / strip shift.

The measurement creation uses center + n * pitch which is correct:

  // Retrieve the specific values based on module idx
    return {mod.pixel.min_center_x + cell.channel0 * mod.pixel.pitch_x,
            mod.pixel.min_center_y + cell.channel1 * mod.pixel.pitch_y};

However, what's filled into the mod.pixel.min_center from the bin utility is min and not min_center.

[krasznaa]

Should this be part of the module description? Should it not rather be the measurement creation code that uses the segmentation information in such a way? 🤔

Since the "segmentation information", I believe, is correct as is. It's how we calculate the measurement locations based on this segmentation info that would need to use the centers of the pixels/stripts. 🤔

[krasznaa]

Crossed with the messages... I'm absolutely on board with shifting the measurement locations like this. Just not with exactly this change. I.e. I would rather change this function instead: https://github.com/acts-project/traccc/blob/main/core/include/traccc/clusterization/impl/measurement_creation.ipp#L18-L24

The CPU and GPU code shares that function, so we should only fix this one function. 😄

[asalzburger]

Crossed with the messages... I'm absolutely on board with shifting the measurement locations like this. Just not with exactly this change. I.e. I would rather change this function instead: https://github.com/acts-project/traccc/blob/main/core/include/traccc/clusterization/impl/measurement_creation.ipp#L18-L24

The CPU and GPU code shares that function, so we should only fix this one function. 😄

Hi, absolutely fine as well, we should then rename the module info then:

min_center_x -> min_corner_x 
min_center_y -> min_corner_y

What do you think?

[krasznaa]

Yes. Let's rename the variables indeed. I only realized that myself after sending the previous message. 🤔

I think there aren't a ridiculous number of clients of those variables, so updating their names should not be a huge undertaking.

[asalzburger]

Good, @fredevb - can you go ahead and make the changes, please?

[fredevb]

I've made the changes :) Would it be possible to make a new release so that I can include the changes in Acts?

[krasznaa]

Ahh... I did not expect the unit tests to go belly up. 😦 Can you investigate Fred?

[stephenswat]

The CCL results are now all offset by a value of 0.5 so they no longer match the expected values.

[krasznaa]

The CCL results are now all offset by a value of 0.5 so they no longer match the expected values.

Any pointers for Fred for what he should update to account for this shift in the tests?

[stephenswat]

The easiest way to update this would be to edit https://github.com/acts-project/traccc/blob/main/tests/common/tests/cca_test.hpp on lines 128, 129, 134, and 135.

[stephenswat]

Note that that value of 0.5 would need to be updated for the pitch of the modules, but these are always assumed to be 1 in the tests so 0.5 should work fine.

[krasznaa]

🤔 How about we just update this line?

https://github.com/acts-project/traccc/blob/main/tests/common/tests/cca_test.hpp#L101

I guess

        for (std::size_t i = 0; i < modules.size(); i++) {
            modules.at(i).pixel = traccc::pixel_data{-0.5f, -0.5f, 1.f, 1.f};
        }

should work, right? 🤔

[krasznaa]

I'll be damned... 😲 This does visibly help our track finding efficiency on the ODD.

With the code from #582 (without this PR's fix), I see:

With this PR put on top, I get:

I.e. there is a small, but visible improvement. 😄

[krasznaa]

I'm cheating, since I'm now approving some of my own code, but c'est la vie...

[asalzburger]

I'll be damned... 😲 This does visibly help our track finding efficiency on the ODD.

With the code from #582 (without this PR's fix), I see:

With this PR put on top, I get:

I.e. there is a small, but visible improvement. 😄

Ha, shall I say "told you so" ... or would that just say I am very old and have been working on this for all too long?

[asalzburger]

By looking at it, I saw that we still have some areas of improvement to make in the cluster error positions.