[RFC] Cellular Automaton: Add hit-compatibility parameters tunable by layer triplets #27955

kjpena · 2019-09-09T14:00:13Z

PR description:

This PR adds classes needed for tuning hit-compatibility cuts of Cellular Automaton for each seeding layer triplet. More details in:
https://indico.cern.ch/event/845939/contributions/3552658/attachments/1904226/3144492/CACuts_KPena_2019-09-09.pdf

PR validation:

Workflow 10824.1 was used to check the performance, starting from step3 with RelVal sample
/RelValTTbar_13/CMSSW_11_0_0_pre6-PU25ns_110X_upgrade2018_realistic_v3-v1/GEN-SIM-DIGI-RAW.

Tracking validation plots are available here:
https://kjpena.web.cern.ch/kjpena/trackingTasks/CMSSW_11_0_0_pre6/

Request for comments from @felicepantaleo, @mtosi, @VinInn, @JanFSchulte

cmsbuild · 2019-09-09T14:00:42Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-09-09T14:06:15Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27955/11817

This PR adds an extra 36KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27955/11817/code-format.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27955/11817/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2019-09-09T14:33:58Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-09-09T14:40:38Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27955/11819

This PR adds an extra 36KB to repository

cmsbuild · 2019-09-09T14:41:09Z

A new Pull Request was created by @kjpena for master.

It involves the following packages:

RecoPixelVertexing/PixelTriplets

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @rovere, @VinInn, @ebrondol, @dgulhan this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

felicepantaleo · 2019-09-09T14:46:03Z

please test

cmsbuild · 2019-09-09T14:46:27Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2436/console Started: 2019/09/09 16:48

cmsbuild · 2019-09-09T16:15:31Z

+1
Tested at: 71ad700
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-823303/2436/summary.html

cmsbuild · 2019-09-09T16:15:34Z

Comparison job queued.

makortel · 2019-09-09T14:49:03Z

RecoPixelVertexing/PixelTriplets/src/CACell.h

-                            const float thetaCut,
-                            const float phiCut,
+                            CACut::CAValuesByInnerLayerIds thetaCutByInnerLayer,
+                            CACut::CAValuesByInnerLayerIds phiCutByInnerLayer,


These seem to result in copying of 2x2 std::vectors, would passing by const reference work?

makortel · 2019-09-09T14:49:18Z

RecoPixelVertexing/PixelTriplets/src/CACell.h

-                            const float thetaCut,
-                            const float phiCut,
+                            CACut::CAValuesByInnerLayerIds thetaCutByInnerLayer,
+                            CACut::CAValuesByInnerLayerIds phiCutByInnerLayer,


Pass by const reference?

makortel · 2019-09-09T14:50:50Z

RecoPixelVertexing/PixelTriplets/src/CACell.h

-                                    const float thetaCut,
-                                    const float phiCut,
+                                    CACut::CAValuesByInnerLayerIds thetaCutByInnerLayer,
+                                    CACut::CAValuesByInnerLayerIds phiCutByInnerLayer,


Pass by const reference?

makortel · 2019-09-09T14:53:35Z

RecoPixelVertexing/PixelTriplets/src/CACut.h

+
+        // Get layer ID
+        thisCACut.layerIds.emplace_back(caLayers.getLayerId(layerName));
+        if (thisCACut.layerIds.at(thisLayer) == -1) {


thisCACut.layerIds.back() would be more idiomatic

makortel · 2019-09-09T15:00:44Z

RecoPixelVertexing/PixelTriplets/src/CAGraph.h

@@ -6,14 +6,19 @@
 #include <vector>

 struct CALayer {
-  CALayer(const std::string &layerName, std::size_t numberOfHits) : theName(layerName) {
+  CALayer(const std::string &layerName, const int &seqNum, std::size_t numberOfHits)


Passing ints by const reference is unnecessary, use either int or const int.

makortel · 2019-09-09T16:35:06Z

RecoPixelVertexing/PixelTriplets/src/CellularAutomaton.cc

-                                     const float thetaCut,
-                                     const float phiCut,
+                                     CACut thetaCut,  //const float thetaCut,
+                                     CACut phiCut,    //const float phiCut,


CACut has internal vectors, pass by const reference?

makortel · 2019-09-09T16:39:05Z

RecoPixelVertexing/PixelTriplets/src/CACut.h

+      thisCACut.cutValue = thisTriplet.cutValue;
+
+      // Add to vector
+      valuesByLayerIds_.emplace_back(thisCACut);


Suggested change

valuesByLayerIds_.emplace_back(thisCACut);

valuesByLayerIds_.emplace_back(std::move(thisCACut));

would avoid copying the std::vector inside thisCACut. Alternatively you could add the element to the vector at the very beginning of the for-loop body by

valuesByLayerIds_.emplace_back(); auto& thisCACut = valuesByLayerIds_.back();

Apparently the capacity of valuesByLayerIds_ is known before the loop, so valuesByLayerIds_.reserve(valuesByTripletNames_); would reduce memory allocations. (maybe even resize() and accessing the elements by index would work?)

makortel · 2019-09-09T16:39:48Z

RecoPixelVertexing/PixelTriplets/src/CACut.h

+        if (thisCACut.layerIds.at(thisLayer) == -1) {
+          edm::LogWarning("Configuration")
+              << "Layer name '" << layerName
+              << "' not found in the CAGraph. Please enter a valid layer name in the CACuts parameter set";


What happens later if thisCACut.layerIds holds -1?

I'm just thinking whether this error should be an exception.

makortel · 2019-09-09T16:41:04Z

RecoPixelVertexing/PixelTriplets/src/CACut.h

+
+      // Layer names and id's
+      std::string layerName;
+      std::size_t layerPos = 0;


There seems to be no use of layerName or layerPos outside of the following loop, so the declarations would be better moved inside the loop as well.

makortel · 2019-09-09T16:49:01Z

RecoPixelVertexing/PixelTriplets/src/CACut.h

+        cutsByInnerLayer.layerIds.emplace_back(thisCut.layerIds[0]);
+        cutsByInnerLayer.cutValues.emplace_back(thisCut.cutValue);
+      }
+    }


How big can the cutsByInnerLayer grow? If it is relatively small, e.g. edm::VecArray would help to reduce memory allocations (since the getCutsByInnerLayer() seems to be called many times within the CA).

Actually, since for given layerIds1, layerIds2 the cutsByInnerLayer is always the same, and the values are known at the time valuesByLayerIds_ are constructed, how about just creating the structure mapping (layerIds1, layerIds2) to cutsByInnerLayer once and returning it here by const reference? (with either a predefined empty value or an error for "not found" cases)

cmsbuild · 2019-09-09T18:05:35Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 7058 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2957224
DQMHistoTests: Total failures: 9818
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2947065
DQMHistoTests: Total skipped: 341
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 145 log files, 15 edm output root files, 34 DQM output files

perrotta · 2019-09-14T10:45:42Z

RecoPixelVertexing/PixelTriplets/src/CAGraph.h

@@ -42,6 +48,14 @@ struct CALayerPair {
 };

 struct CAGraph {
+  int getLayerId(const std::string &layerName) {
+    for (auto thisLayer = theLayers.begin(); thisLayer != theLayers.end(); thisLayer++) {


This can be a range based for

perrotta · 2019-09-14T10:46:00Z

RecoPixelVertexing/PixelTriplets/src/CACut.h

+  class CAValuesByInnerLayerIds {
+  public:
+    float at(int layerId) {
+      for (size_t thisLayer = 0; thisLayer < layerIds.size(); thisLayer++) {


This can be a range based for

perrotta · 2019-09-16T13:22:15Z

There are quite a lot of differences in Phase2 outputs, which was not what planned I guess: please have a look

perrotta · 2019-09-26T12:24:45Z

@kjpena , which is the status of the checks about the Phase2 related issues, and the plans for updating this PR according to the first sets of comments received?

perrotta · 2019-10-03T11:38:54Z

@kjpena: is there any news? any plan?

perrotta · 2019-10-14T17:12:41Z

-1
(Unattended since more than one month, we will reconsider this once updated and comments answered)

fabiocos · 2019-10-30T09:10:38Z

@kjpena please clarify the status of this PR, is there work planned in next future?

kjpena · 2019-11-04T20:26:21Z

@fabiocos @perrotta I need one more week to wrap up another project. Then I'm back to implementing the comments received. My apologies for the long delay.

fabiocos · 2019-12-16T15:37:34Z

@kjpena is this PR still active, or may be it is better to close it a restart with a fresh new approach as soon as there is time to work on it?

fabiocos · 2019-12-16T15:37:39Z

hold

cmsbuild · 2019-12-16T15:38:01Z

Pull request has been put on hold by @fabiocos
They need to issue an unhold command to remove the hold state or L1 can unhold it for all

silviodonato · 2020-01-08T13:57:33Z

I'm closing the PRs with no activity in the last months. @kjpena please open it again as soon as you have news.

silviodonato · 2020-01-08T13:57:41Z

please close

kjpena added 5 commits July 31, 2019 12:49

Adding CACuts

3199297

New release

8e0782d

Removing unnecessary comments

f41e329

Removing unnecessary comments

8f301a8

Updating CACuts

4f888cd

cmsbuild added this to the CMSSW_11_0_X milestone Sep 9, 2019

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Sep 9, 2019

cmsbuild added code-checks-rejected and removed code-checks-pending labels Sep 9, 2019

Applying code-format patchs

71ad700

cmsbuild added code-checks-pending and removed code-checks-rejected labels Sep 9, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 9, 2019

cmsbuild added tests-started and removed tests-pending labels Sep 9, 2019

makortel reviewed Sep 9, 2019

View reviewed changes

cmsbuild added comparison-available and removed comparison-pending labels Sep 9, 2019

cmsbuild mentioned this pull request Sep 12, 2019

Squash all Patatrack developments on top of CMSSW_11_3_0_pre5 #27983

Closed

perrotta reviewed Sep 14, 2019

View reviewed changes

kjpena changed the title ~~[RFC] Cellular Automaton: Add hit-compatibility parameters tunable by layer triplets~~ Cellular Automaton: Add hit-compatibility parameters tunable by layer triplets Sep 20, 2019

cmsbuild added reconstruction-rejected and removed reconstruction-pending labels Oct 14, 2019

smuzaffar modified the milestones: CMSSW_11_0_X, CMSSW_11_1_X Dec 2, 2019

cmsbuild modified the milestones: CMSSW_11_1_X, CMSSW_11_0_X Dec 10, 2019

cmsbuild added the hold label Dec 16, 2019

cmsbuild closed this Jan 8, 2020

kjpena changed the title ~~Cellular Automaton: Add hit-compatibility parameters tunable by layer triplets~~ [RFC] Cellular Automaton: Add hit-compatibility parameters tunable by layer triplets Apr 20, 2020

kjpena mentioned this pull request May 7, 2020

Cellular Automaton: Add hit-compatibility parameters tunable by layer triplets #29760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Cellular Automaton: Add hit-compatibility parameters tunable by layer triplets #27955

[RFC] Cellular Automaton: Add hit-compatibility parameters tunable by layer triplets #27955

kjpena commented Sep 9, 2019

cmsbuild commented Sep 9, 2019

cmsbuild commented Sep 9, 2019

cmsbuild commented Sep 9, 2019

cmsbuild commented Sep 9, 2019

cmsbuild commented Sep 9, 2019

felicepantaleo commented Sep 9, 2019

cmsbuild commented Sep 9, 2019 •

edited

Loading

cmsbuild commented Sep 9, 2019

cmsbuild commented Sep 9, 2019

makortel Sep 9, 2019

makortel Sep 9, 2019

makortel Sep 9, 2019

makortel Sep 9, 2019

makortel Sep 9, 2019

makortel Sep 9, 2019

makortel Sep 9, 2019

makortel Sep 9, 2019

makortel Sep 9, 2019

makortel Sep 9, 2019

cmsbuild commented Sep 9, 2019

perrotta Sep 14, 2019

perrotta Sep 14, 2019

perrotta commented Sep 16, 2019 •

edited

Loading

perrotta commented Sep 26, 2019

perrotta commented Oct 3, 2019

perrotta commented Oct 14, 2019

fabiocos commented Oct 30, 2019

kjpena commented Nov 4, 2019

fabiocos commented Dec 16, 2019

fabiocos commented Dec 16, 2019

cmsbuild commented Dec 16, 2019

silviodonato commented Jan 8, 2020

silviodonato commented Jan 8, 2020

	valuesByLayerIds_.emplace_back(thisCACut);
	valuesByLayerIds_.emplace_back(std::move(thisCACut));

[RFC] Cellular Automaton: Add hit-compatibility parameters tunable by layer triplets #27955

[RFC] Cellular Automaton: Add hit-compatibility parameters tunable by layer triplets #27955

Conversation

kjpena commented Sep 9, 2019

PR description:

PR validation:

cmsbuild commented Sep 9, 2019

cmsbuild commented Sep 9, 2019

cmsbuild commented Sep 9, 2019

cmsbuild commented Sep 9, 2019

cmsbuild commented Sep 9, 2019

felicepantaleo commented Sep 9, 2019

cmsbuild commented Sep 9, 2019 • edited Loading

cmsbuild commented Sep 9, 2019

cmsbuild commented Sep 9, 2019

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

cmsbuild commented Sep 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perrotta commented Sep 16, 2019 • edited Loading

perrotta commented Sep 26, 2019

perrotta commented Oct 3, 2019

perrotta commented Oct 14, 2019

fabiocos commented Oct 30, 2019

kjpena commented Nov 4, 2019

fabiocos commented Dec 16, 2019

fabiocos commented Dec 16, 2019

cmsbuild commented Dec 16, 2019

silviodonato commented Jan 8, 2020

silviodonato commented Jan 8, 2020

cmsbuild commented Sep 9, 2019 •

edited

Loading

perrotta commented Sep 16, 2019 •

edited

Loading