adding fast and parallel-friendly kdtree implementation #18288

felicepantaleo · 2017-04-10T14:45:30Z

This PR adds a fast/parallel-friendly kd-tree implementation in CommonTools.
The plan is to replace the existing PF KDtree (based on lists) with this one which is based on vectors and SoA in HGCal wfs, making it friendly for accelerators.
Searching and building produces exactly the same results, searching with 3D point cloud of about 100k points makes it ~3x faster than the existing implementation.

@rovere @malgeri @VinInn @makortel

cmsbuild · 2017-04-10T14:45:50Z

A new Pull Request was created by @felicepantaleo (Felice Pantaleo) for master.

It involves the following packages:

CommonTools/RecoAlgos

@perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@rappoccio, @jdolen, @makortel, @abbiendi, @jhgoh, @ahinzmann, @gkasieczka this is something you requested to watch as well.
@Muzaffar, @davidlange6, @smuzaffar you are the release manager for this.

cms-bot commands are listed here

slava77 · 2017-04-10T15:19:49Z

@cmsbuild please test

cmsbuild · 2017-04-10T15:20:04Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19056/console Started: 2017/04/10 17:21

cmsbuild · 2017-04-10T16:47:26Z

+1
Tested at: b5d5f86
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18288/19056/summary.html

cmsbuild · 2017-04-10T16:47:30Z

Comparison job queued.

cmsbuild · 2017-04-10T21:15:32Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18288/19056/summary.html

Comparison Summary:

You potentially added 43 lines to the logs
Reco comparison results: 1643 differences found in the comparisons
DQMHistoTests: Total files compared: 23
DQMHistoTests: Total histograms compared: 1917175
DQMHistoTests: Total failures: 23404
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 1893598
DQMHistoTests: Total skipped: 173
DQMHistoTests: Total Missing objects: 0
Checked 94 log files, 14 edm output root files, 23 DQM output files

cmsbuild · 2017-04-10T22:25:52Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18288/19056/summary.html

Comparison Summary:

You potentially added 43 lines to the logs
Reco comparison results: 1643 differences found in the comparisons
DQMHistoTests: Total files compared: 23
DQMHistoTests: Total histograms compared: 1917175
DQMHistoTests: Total failures: 23404
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 1893598
DQMHistoTests: Total skipped: 173
DQMHistoTests: Total Missing objects: 0
Checked 94 log files, 14 edm output root files, 23 DQM output files

perrotta

In general, the huge indentation (it is better avoiding computer tabs for it) and the unnecessary and illogical insertion of empty lines and instructions broken in two or more lines, make the review of this code rather painful.
Could you please adjust it, and make it better readable?

perrotta · 2017-04-21T09:27:58Z

CommonTools/RecoAlgos/interface/FKDTree.h

+	    v |= v >> 8;
+	    v |= v >> 16;
+	    return MultiplyDeBruijnBitPosition[(unsigned int)(v * 0x07C4ACDDU) >> 27];
+


Please improve readability by removing the many empty lines (when not needed to separate methods or logical parts inside the code)

perrotta · 2017-04-21T09:28:41Z

CommonTools/RecoAlgos/interface/FKDTree.h

+	    return MultiplyDeBruijnBitPosition[(unsigned int)(v * 0x07C4ACDDU) >> 27];
+
+	    }
+


Remove till L28

perrotta · 2017-04-21T09:29:16Z

CommonTools/RecoAlgos/interface/FKDTree.h

+	{
+		theNumberOfPoints = 0;
+		theDepth = 0;
+


perrotta · 2017-04-21T09:29:49Z

CommonTools/RecoAlgos/interface/FKDTree.h

+
+	void push_back(const FKDPoint<TYPE, numberOfDimensions>& point)
+	{
+


remove empty lines here and below the instruction

perrotta · 2017-04-21T09:30:04Z

CommonTools/RecoAlgos/interface/FKDTree.h

+
+	void push_back(FKDPoint<TYPE, numberOfDimensions> && point)
+	{
+


remove empty lines here and below the instruction

perrotta · 2017-04-21T09:54:01Z

CommonTools/RecoAlgos/interface/FKDTree.h

+		if (length == 1)
+			return 0;
+		unsigned int index = 1 << (ilog2(length));
+


remove one line

perrotta · 2017-04-21T09:54:28Z

CommonTools/RecoAlgos/interface/FKDTree.h

+		for (unsigned int i = 0; i < numberOfDimensions; ++i)
+			theDimensions[i][position] = point[i];
+		theIds[position] = point.getId();
+


perrotta · 2017-04-21T09:54:41Z

CommonTools/RecoAlgos/interface/FKDTree.h

+		for (unsigned int i = 0; i < numberOfDimensions; ++i)
+			theDimensions[i][position] = point[i];
+		theIds[position] = point.getId();
+


perrotta · 2017-04-21T09:56:22Z

CommonTools/RecoAlgos/interface/FQueue.h

+		theSize = 0;
+		theFront = 0;
+		theTail = 0;
+	}


here one empty line could help, instead...

perrotta · 2017-04-21T09:57:23Z

CommonTools/RecoAlgos/interface/FQueue.h

+		theFront = 0;
+		theTail = 0;
+		theCapacity = 0;
+


The same comments about empty lines and indentations also hold for this file (not marked all them)

VinInn · 2017-04-21T10:36:51Z

Dear @perrotta , I do not disagree on the specific, still we cannot change coding style at each coordinator change.
Is there a (still valid) cms coding rule about spacing and tabbing?

perrotta · 2017-04-21T10:54:06Z

Ciao @VinInn
Honestly, I don't know if there there are specific official rules about spacing and tabbing in CMSSW. I only found this piece of code extremely difficult to read and follow on, mostly because of the huge indentations (that force in a few cases to break the same instruction into separate lines) and the wide usage of scattered empty lines. For a possible example of a much better readable code I could cite your own 18408 (just the first example I got at hand)

slava77 · 2017-04-21T11:54:49Z

On 4/21/17 3:36 AM, Vincenzo Innocente wrote: Dear @perrotta <https://github.com/perrotta> , I do not disagree on the specific, still we cannot change coding style at each coordinator change.

I'm curious who was the last coordinator that asked you to code with 8-char indentation or add extra empty lines inside function scopes.

…

Is there a (still valid) cms coding rule about spacing and tabbing? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18288 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEdcbkkoZJLnbDwYTaG52-wf0hq90AKBks5ryIbEgaJpZM4M44fI>.

kpedro88 · 2017-04-25T17:03:03Z

CommonTools/RecoAlgos/interface/FKDTree.h

+	}
+
+
+	void build()


The total duplication of the build() function just to change the source of the points vector is unnecessary. This function only needs to exist once.

cmsbuild · 2017-05-08T12:36:28Z

Pull request #18288 was updated. @perrotta, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

felicepantaleo · 2017-05-08T12:38:33Z

@perrotta could you point me to unit tests of some other data structure so that I can imitate?

cmsbuild · 2017-05-08T16:07:14Z

Pull request #18288 was updated. @perrotta, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

perrotta · 2017-05-09T09:41:10Z

please test

cmsbuild · 2017-05-09T09:41:30Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/19667/console Started: 2017/05/09 11:43

cmsbuild · 2017-05-09T11:54:58Z

+1
Tested at: c5ceeae
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18288/19667/summary.html

cmsbuild · 2017-05-09T11:55:02Z

Comparison job queued.

cmsbuild · 2017-05-09T15:30:28Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-18288/19667/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 1669 differences found in the comparisons
DQMHistoTests: Total files compared: 24
DQMHistoTests: Total histograms compared: 1831050
DQMHistoTests: Total failures: 25574
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 1805295
DQMHistoTests: Total skipped: 180
DQMHistoTests: Total Missing objects: 0
Checked 98 log files, 14 edm output root files, 24 DQM output files

perrotta · 2017-05-10T16:03:22Z

+1
New code made available in CommonTools/RecoAlgos, not accessed by any module in CMSSW by now.
Visual inspection looks ok, and the unit test provided (thank you Felice) ends succesfully.
Full debug and complete check of fuctionality will necessarily only be dealt with once this code will be actually used in future applications.

cmsbuild · 2017-05-10T16:03:41Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

davidlange6 · 2017-05-11T07:26:29Z

+1

adding fast and parallel-friendly kdtree implementation

b5d5f86

cmsbuild added this to the Next CMSSW_9_1_X milestone Apr 10, 2017

cmsbuild added comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Apr 10, 2017

cmsbuild added tests-started and removed tests-pending labels Apr 10, 2017

cmsbuild added tests-approved and removed tests-started labels Apr 10, 2017

cmsbuild added comparison-available and removed comparison-pending labels Apr 10, 2017

perrotta reviewed Apr 21, 2017

View reviewed changes

Merged refs/pull/18288/head from repository cms-sw

efd7fcf

kpedro88 reviewed Apr 25, 2017

View reviewed changes

removing internal vector and convert tabs to spaces

d9f6b54

cmsbuild added comparison-pending and removed comparison-available tests-approved labels Apr 28, 2017

cmsbuild added comparison-pending tests-pending and removed comparison-available tests-approved labels May 8, 2017

felicepantaleo added 2 commits May 8, 2017 18:06

improve comments

f0eba99

adding unit test

c5ceeae

cmsbuild added tests-started and removed tests-pending labels May 9, 2017

cmsbuild added tests-approved and removed tests-started labels May 9, 2017

cmsbuild added comparison-available and removed comparison-pending labels May 9, 2017

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels May 10, 2017

cmsbuild added orp-approved and removed orp-pending labels May 11, 2017

cmsbuild merged commit 23b3aeb into cms-sw:master May 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding fast and parallel-friendly kdtree implementation #18288

adding fast and parallel-friendly kdtree implementation #18288

felicepantaleo commented Apr 10, 2017

cmsbuild commented Apr 10, 2017 •

edited by smuzaffar

slava77 commented Apr 10, 2017

cmsbuild commented Apr 10, 2017 •

edited

cmsbuild commented Apr 10, 2017

cmsbuild commented Apr 10, 2017

cmsbuild commented Apr 10, 2017

cmsbuild commented Apr 10, 2017

perrotta left a comment

perrotta Apr 21, 2017

perrotta Apr 21, 2017

perrotta Apr 21, 2017

perrotta Apr 21, 2017

perrotta Apr 21, 2017

perrotta Apr 21, 2017

perrotta Apr 21, 2017

perrotta Apr 21, 2017

perrotta Apr 21, 2017

perrotta Apr 21, 2017

VinInn commented Apr 21, 2017

perrotta commented Apr 21, 2017

slava77 commented Apr 21, 2017 via email

kpedro88 Apr 25, 2017

cmsbuild commented May 8, 2017

felicepantaleo commented May 8, 2017

cmsbuild commented May 8, 2017

perrotta commented May 9, 2017

cmsbuild commented May 9, 2017 •

edited

cmsbuild commented May 9, 2017

cmsbuild commented May 9, 2017

cmsbuild commented May 9, 2017

perrotta commented May 10, 2017

cmsbuild commented May 10, 2017

davidlange6 commented May 11, 2017

		return MultiplyDeBruijnBitPosition[(unsigned int)(v * 0x07C4ACDDU) >> 27];

		}


		void push_back(const FKDPoint<TYPE, numberOfDimensions>& point)
		{


		void push_back(FKDPoint<TYPE, numberOfDimensions> && point)
		{

adding fast and parallel-friendly kdtree implementation #18288

adding fast and parallel-friendly kdtree implementation #18288

Conversation

felicepantaleo commented Apr 10, 2017

cmsbuild commented Apr 10, 2017 • edited by smuzaffar

slava77 commented Apr 10, 2017

cmsbuild commented Apr 10, 2017 • edited

cmsbuild commented Apr 10, 2017

cmsbuild commented Apr 10, 2017

cmsbuild commented Apr 10, 2017

cmsbuild commented Apr 10, 2017

perrotta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinInn commented Apr 21, 2017

perrotta commented Apr 21, 2017

slava77 commented Apr 21, 2017 via email

Choose a reason for hiding this comment

cmsbuild commented May 8, 2017

felicepantaleo commented May 8, 2017

cmsbuild commented May 8, 2017

perrotta commented May 9, 2017

cmsbuild commented May 9, 2017 • edited

cmsbuild commented May 9, 2017

cmsbuild commented May 9, 2017

cmsbuild commented May 9, 2017

perrotta commented May 10, 2017

cmsbuild commented May 10, 2017

davidlange6 commented May 11, 2017

cmsbuild commented Apr 10, 2017 •

edited by smuzaffar

cmsbuild commented Apr 10, 2017 •

edited

cmsbuild commented May 9, 2017 •

edited