Skip to content

Conversation

@mpuccio
Copy link
Contributor

@mpuccio mpuccio commented Apr 12, 2021

Hi @shahor02 @iouribelikov,
I recovered quite some efficiency but I am still investigating a random crash. I open this in draft while I fix that.
Cheers,
Max

@mpuccio mpuccio requested review from a team, bovulpes and iouribelikov as code owners April 12, 2021 09:38
@mpuccio mpuccio marked this pull request as draft April 12, 2021 09:38
@mpuccio mpuccio marked this pull request as ready for review May 11, 2021 14:55
@mpuccio mpuccio requested a review from rpezzi as a code owner May 11, 2021 14:55
mpuccio added a commit to mpuccio/AliceO2 that referenced this pull request May 11, 2021
Please consider the following formatting changes to AliceO2Group#5911
@mpuccio mpuccio changed the title [WIP] Time frame based ITS tracking Time frame based ITS tracking Jun 18, 2021
@mpuccio
Copy link
Contributor Author

mpuccio commented Jun 18, 2021

Hi @shahor02 @iouribelikov , this is finally ready to be reviewed/merged. I still have to add the bit for flagging the tracks with clusters on more than one ROF, but this should be faster than this large modification.

@mpuccio
Copy link
Contributor Author

mpuccio commented Jun 21, 2021

@mconcas the last commit should fix the compilation of the ALICE3 macro. However, the functionality is not there: the tf are not properly filled with clusters. One should add an event by event fill method for the time frame to make it work

@mpuccio
Copy link
Contributor Author

mpuccio commented Jun 28, 2021

Hi @iouribelikov, now one can disable the multi-rof reconstruction by setting deltaROF=0 in the configuration of the tracker. If fine with you I would merge this, such that with Matteo we can synchronise the next developments

@iouribelikov
Copy link
Collaborator

Hi @iouribelikov, now one can disable the multi-rof reconstruction by setting deltaROF=0 in the configuration of the tracker. If fine with you I would merge this, such that with Matteo we can synchronise the next developments

Ciao, @mpuccio. In general, I do not have anything against merging these two PRs. It's just I do not see much improvement in the efficiency and fake-track rate.

Can we get a "snapshot" of the current performance (efficiency, fakes, CPU, memory), with the current best values for all tracking parameters ? I think, it is important to be sure that we at least do not degrade our performance, after the merging is done.

@mpuccio
Copy link
Contributor Author

mpuccio commented Jun 28, 2021

OK, for pp using your 1000 pp events with this PR I get:

	Command being timed: "o2-its-reco-workflow --trackerCA --tracking-mode async --configKeyValues ITSVertexerParam.phiCut=0.5;ITSVertexerParam.clusterContributorsCut=3;ITSVertexerParam.tanLambdaCut=0.2 -b"
	User time (seconds): 9.91
	System time (seconds): 2.83
	Percent of CPU this job got: 241%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:05.28
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 2734340
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 0
	Minor (reclaiming a frame) page faults: 1110460
	Voluntary context switches: 33931
	Involuntary context switches: 1304
	Swaps: 0
	File system inputs: 0
	File system outputs: 42768
	Socket messages sent: 0
	Socket messages received: 0
	Signals delivered: 0
	Page size (bytes): 4096
	Exit status: 0

Corresponding to:
Good found tracks/event: 4, efficiency: 0.939128, fake-track rate: 0.0675035 clone rate 0.0168167

With the current dev I get:

	Command being timed: "o2-its-reco-workflow --trackerCA --tracking-mode async --configKeyValues ITSVertexerParam.phiCut=0.5;ITSVertexerParam.clusterContributorsCut=3;ITSVertexerParam.tanLambdaCut=0.2 -b"
	User time (seconds): 8.50
	System time (seconds): 2.31
	Percent of CPU this job got: 251%
	Elapsed (wall clock) time (h:mm:ss or m:ss): 0:04.29
	Average shared text size (kbytes): 0
	Average unshared data size (kbytes): 0
	Average stack size (kbytes): 0
	Average total size (kbytes): 0
	Maximum resident set size (kbytes): 837976
	Average resident set size (kbytes): 0
	Major (requiring I/O) page faults: 3
	Minor (reclaiming a frame) page faults: 623453
	Voluntary context switches: 40483
	Involuntary context switches: 1085
	Swaps: 0
	File system inputs: 536
	File system outputs: 42736
	Socket messages sent: 0
	Socket messages received: 0
	Signals delivered: 0
	Page size (bytes): 4096
	Exit status: 0

and for the efficiency:
Good found tracks/event: 4, efficiency: 0.942444, fake-track rate: 0.0563714 clone rate 0.0184747

Summarising: we use more memory, we are 25% slower and the efficiencies are comparable (slightly lower efficiency with the PR, higher fakes and reduced clones)

@iouribelikov
Copy link
Collaborator

@mpuccio Thank you very much, Max for these important numbers !
Look, I would suggest merging these two PRs. So that we can move ahead with the code development.
( If it is OK with @shahor02. )

iouribelikov
iouribelikov previously approved these changes Jun 28, 2021
@shahor02
Copy link
Collaborator

OK for me once the tests passed. But, @mpuccio , is it clear why there is no improvement in the eff.?

@mpuccio
Copy link
Contributor Author

mpuccio commented Jun 28, 2021

@shahor02, not really. But I'm also not that convinced that we really lose 4% out of this effect: the efficiency was always saturating at 1 at high pt with high statistical accuracy (better than 4%). Currently CheckTracks.C puts at denominator particles that have at least a cluster in all layers, and it does it, if I am not mistaken, looking also across different ROFs, thus the inefficiency should have been already evident with the current dev.

@shahor02
Copy link
Collaborator

How much we can lose depends on the eventual strobe length (the better/shorter it is the more we can lose) and the signal strength (since the rise time depends on it). In any case, we need to evaluate the eff. as a function of the event distance to the strobe boundary. If I got it right, @iouribelikov tries to get it?

@mpuccio mpuccio closed this Jun 28, 2021
@mpuccio mpuccio reopened this Jun 28, 2021
@iouribelikov
Copy link
Collaborator

iouribelikov commented Jun 28, 2021

@mpuccio The CheckTracks.C is probably not the best tool to study this effect. The natural variable would be the time difference between the boundaries of ALPIDE strobes and the moment an interaction happens, as was suggested by @shahor02.
I already gave it a try, and the results were not easy to understand (for me). Everything could still be OK, but I'd need to digest it.
Now, since this option is anyway optional, let's merge it. So that @mconcas could move on.

@mpuccio
Copy link
Contributor Author

mpuccio commented Jun 29, 2021

@shahor02 @davidrohr, I am not sure to understand why the fullCI is breaking on TrackITS and MCCompLabel while the normal o2 build (and my local build) works fine. Do you have any insights?

@shahor02
Copy link
Collaborator

@mpuccio HIP compilation complains on o2::dataformats::RangeRefComp not having default c-tor. It does have it but it is defined as GPUdDefault(). If I get it right, with HIP it will become __device__, i.e. will not be available on the host?
Do you try locally with HIP? Perhaps @davidrohr may suggest something?

@mpuccio mpuccio force-pushed the tf-tracking branch 2 times, most recently from b30cabe to 4e575b5 Compare August 30, 2021 18:47
@mpuccio
Copy link
Contributor Author

mpuccio commented Sep 2, 2021

Hi @shahor02, unless there are surprises in the latest CIs or any objections from you, I would merge the PR here as it is

@mpuccio
Copy link
Contributor Author

mpuccio commented Sep 3, 2021

CI fully green :)

@shahor02 shahor02 merged commit 8069b93 into AliceO2Group:dev Sep 3, 2021
@mpuccio mpuccio deleted the tf-tracking branch September 3, 2021 11:42
shahor02 added a commit to shahor02/AliceO2 that referenced this pull request Sep 9, 2021
This reverts commit 8069b93.

Since it breaks ITS track cluster indices container:
https://alice.its.cern.ch/jira/browse/O2-2571
shahor02 added a commit that referenced this pull request Sep 9, 2021
This reverts commit 8069b93.

Since it breaks ITS track cluster indices container:
https://alice.its.cern.ch/jira/browse/O2-2571
mpuccio added a commit to mpuccio/AliceO2 that referenced this pull request Sep 10, 2021
shahor02 pushed a commit that referenced this pull request Sep 13, 2021
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.

5 participants