Skip to content

[ITS] Remove custom boilerplate in favour of MCCompLabel - #2235

Merged
davidrohr merged 2 commits into
AliceO2Group:devfrom
mconcas:fix-mccomplabels
Jul 20, 2019
Merged

[ITS] Remove custom boilerplate in favour of MCCompLabel#2235
davidrohr merged 2 commits into
AliceO2Group:devfrom
mconcas:fix-mccomplabels

Conversation

@mconcas

@mconcas mconcas commented Jul 19, 2019

Copy link
Copy Markdown
Collaborator

Hi @shahor02, Anne,
this will remove my custom labels in favour of the MCComplLabel mainstream ones.
This will improve the consistency of code.

GPU code for the moment is still using my custom ones.
I'll sort it out in the upcoming reiteration on the GPU code I'm going to perform next.

@a-kieffer for the record, as soon as we get new simulated data the check on noise will be consistent.

Cheers
Matteo

@mconcas
mconcas requested review from a team and ktf as code owners July 19, 2019 14:38
@mconcas mconcas changed the title Remove custom boilerplate in favour of MCCompLabel [ITS] Remove custom boilerplate in favour of MCCompLabel Jul 19, 2019
@mconcas
mconcas force-pushed the fix-mccomplabels branch from f160ae2 to 74fbf4e Compare July 19, 2019 14:53
for (int iNextLayerClusterIndex{ firstRowClusterIndex }; iNextLayerClusterIndex < maxRowClusterIndex && iNextLayerClusterIndex < (int)clustersNextLayer.size(); ++iNextLayerClusterIndex) {
const Cluster& nextCluster{ clustersNextLayer[iNextLayerClusterIndex] };
const char testMC{ !isMc || (nextLayerMClabels[iNextLayerClusterIndex] == currentLayerMClabels[iCurrentLayerClusterIndex] && nextLayerMClabels[iNextLayerClusterIndex] != -1) };
const auto& lblNext = evt->getClusterLabels(layerIndex, nextCluster.clusterId);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without checking in detail, I would where possible try to keep the reconstruction code and the MC code separate. Don't know if this is possible here, but e.g. for the TPC I first create the tracks, then if MC is present I loop over the tracks and assign the labels. In addition I have some debug code inside the tracking that might access the labels, but that is usually disabled at compile time.

@mconcas mconcas Jul 20, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see what you mean.
For the moment the upstream code is giving wrong results as a consequence of the changes to labels.
I'd rather merge this in order to make code behave correctly.
Later I'll factorise the MC part out from the reco code, maybe changing it in a way it will work also with the GPU version.

Cheers

@davidrohr davidrohr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mconcas : Agreed, we can merge this once you fix the one problem I commented (which the CI currently does not check).

We can then thin together (also with @mpuccio ) how to handle MC labels in GPU code.

for (auto& trklet01 : mComb01) {
for (auto& trklet12 : mComb12) {
if (trklet01.secondClusterIndex == trklet12.firstClusterIndex) {
const float deltaTanLambda{ gpu::GPUCommonMath::Abs(trklet01.tanLambda - trklet12.tanLambda) };

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mconcas : this gives me a build error with -Werror (apparently the CI didn't catch it, probably the cmake migration broke this, investigating). Anyway, can you move the definition inside the #if defined protection?

@davidrohr
davidrohr merged commit 5f93282 into AliceO2Group:dev Jul 20, 2019
MichaelLettrich pushed a commit that referenced this pull request Jul 24, 2019
* Use MCompLabels in place of custom boilerplate

* Fix unspotted warning
knopers8 pushed a commit to knopers8/AliceO2 that referenced this pull request Oct 23, 2019
…p#2235)

* Use MCompLabels in place of custom boilerplate

* Fix unspotted warning
carlos-soncco pushed a commit to carlos-soncco/AliceO2 that referenced this pull request Oct 28, 2019
…p#2235)

* Use MCompLabels in place of custom boilerplate

* Fix unspotted warning
@mconcas
mconcas deleted the fix-mccomplabels branch February 17, 2021 08:31
EmilGorm pushed a commit to EmilGorm/AliceO2 that referenced this pull request Apr 15, 2023
* propagated tracks

* propagated tracks used in all histos

* typo

* clang

* add more control histos

* after review edit and new histogram added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants