Candidate ranking à la CMSSW #167

mmasciov · 2018-10-02T15:31:51Z

https://mmasciov.web.cern.ch/mmasciov/benchmarks_cmsswranking_forPR/

makortel · 2018-10-02T15:51:24Z

mkFit/CandCloner.cc

+    if(pt[c]<0.9f) score[c] -= 0.5f*(validHitBonus_)*nfoundhits[c];
+    else if(nfoundhits[c]>8) score[c] += (validHitBonus_)*nfoundhits[c];
+  }
+  return score[0]>score[1];


The logic is repeated four times. Would it be possible to reduce the copy-paste, e.g. for those having a Track object calling the sortByScore(const Track&, const Track&)), and here where that is not possible, abstract the score calculation (=for loop) to an inlined function that is then called also from sortByScore()?

I would agree with Matti here, definitely the less copy-paste the better :)

makortel · 2018-10-02T15:52:10Z

mkFit/CandCloner.cc

+
+
+  float validHitBonus_=2.5;
+  float missingHitPenalty_=20.0;


Why hardcode the constants here instead of taking them via Config? (same for sortCandByScore() in `MkBuilder.cc)

These values are actually already sitting in Config.h. So, these two lines are actually not needed.

kmcdermo · 2018-10-08T00:18:15Z

Hey Mario, I have a few comments:

It looks like the SNB tests failed... And it looks like you managed to escape the crash in STD building with SIMVAL... which makes it hard to compare with the latest baselines. Can you run the benchmarks again (using 100 events for the validation - see Issue Fixing up of SlurpIn #168 diff)?
Or, just for comparisons sake for the validation, can you link an old set of plots (pre this PR)? Or perhaps the slides you presented in the group meeting?
I would agree with Matti that it looks like this code can be refactored -- and the less copy/paste, the easier it is to maintain.

mmasciov · 2018-10-11T18:26:18Z

Hi Matti, Kevin,

@makortel I have tried to 'welcome' your suggestions, and rearranged the code.
@kmcdermo I don't know why SNB fails, and sometimes KNL too. And many times I get "random" core dumps, either for STD or CE SIMVAL (both in the PR scope, and in the original scope).
I reran both original and PR code with 250 events (running with 100 is sub-optimal, as the improvements may be eaten by statistics):
Original: https://mmasciov.web.cern.ch/mmasciov/benchmarks_originalranking_250evts/
PR: https://mmasciov.web.cern.ch/mmasciov/benchmarks_cmsswranking_250evts/
Still, there are some holes (SNB and/or KNL), but at least on SKL-SP everything was successful, so it's still ok to look and have an apple-to-apple comparison.

makortel · 2018-10-11T18:30:18Z

Track.h

+  int nmisshits[2] = {cand1.first.nTotalHits()-cand1.first.nFoundHits(),cand2.first.nTotalHits()-cand2.first.nFoundHits()};
+  float chi2[2] = {cand1.first.chi2(),cand2.first.chi2()};
+  float pt[2] = {cand1.first.pT(),cand2.first.pT()};
+  return sortByScoreLoop(nfoundhits,nmisshits,chi2,pt);


Thanks @mmasciov. Would return sortByScoreCand(cand1.first, cand2.first); work here?

Yes, why not. I have fixed this, and compilation doesn't complain. Thanks.

kmcdermo · 2018-10-11T19:23:21Z

@mmasciov , thanks for the cleanups -- looks nice! As for the crashes in the validation: this is a known issue with SlurpIn when running too many events with lots of events in flight.

We can skirt around the crash if we run fewer events in the validation (when using MEIF). OR, if you are concerning about the stats, we can keep the number of events the same, but then disable the MEIF validation:

[macbook] mictest > git diff val_scripts/validation-cmssw-benchmarks.sh
diff --git a/val_scripts/validation-cmssw-benchmarks.sh b/val_scripts/validation-cmssw-benchmarks.sh
index 595f388..9fb8728 100755
--- a/val_scripts/validation-cmssw-benchmarks.sh
+++ b/val_scripts/validation-cmssw-benchmarks.sh
@@ -22,7 +22,7 @@ nevents=500
 ## Common executable setup
 maxth=64
 maxvu=16
-maxev=32
+maxev=1
 seeds="--cmssw-n2seeds"
 exe="./mkFit/mkFit --silent ${seeds} --num-thr ${maxth} --num-thr-ev ${maxev} --input-file ${dir}/${subdir}/${file} --num-events ${nevents}"
 
@@ -67,9 +67,6 @@ function doVal()
     echo "${oBase}: ${vN} [nTH:${maxth}, nVU:${maxvu}int, nEV:${maxev}]"
     ${bExe} >& log_${oBase}_NVU${maxvu}int_NTH${maxth}_NEV${maxev}_${vN}.txt
     
-    # hadd output files for this test, then move to temporary directory
-    hadd -O valtree.root valtree_*.root
-    rm valtree_*.root
     mv valtree.root ${tmpdir}/valtree_${oBase}_${vN}.root
 }

So, could you try running the validation with 500 events, but turning off MEIF?

kmcdermo · 2018-10-11T19:24:24Z

The crashes on SNB are concerning... can you post the logs of one of the SNB tests?

mmasciov · 2018-10-11T19:33:30Z

As for the crashes with large number of events: 250 events work fine, and it's enough for my purposes.

As for the crashes on SNB: note that they happen even without changing a comma in the code, freshly cloned. I can submit another benchmark, and point do not clean the logs (when I move the benchmarks on lxplus).

kmcdermo · 2018-10-11T19:51:36Z

Huh. Okay, well, then that is really concerning. MT's PR also showed crashes on KNL...

mmasciov · 2018-10-12T12:40:35Z

For SNB, the logs do not help (me) much:

less benchmark_snb_dump.txt
Executing SNB tests remotely...
bash: line 1: cd: /data2/nfsmic/mmasciov/tmp: No such file or directory
bash: line 2: ./xeon_scripts/benchmark-cmssw-ttbar-fulldet-build.sh: No such file or directory
Copying logs back from SNB for plotting
scp: /data2/nfsmic/mmasciov/tmp/log_SNB_CMSSW_TTbar_PU70_*.txt: No such file or directory
Removing tmp dir on SNB remotely
less log_SNB_CMSSW_TTbar_PU70_BH_TH.txt
grep: log_SNB_CMSSW_TTbar_PU70_BH_NVU8int_NTH24.txt: No such file or directory

mmasciov · 2018-10-12T12:53:25Z

After looking into this, I think there's an issue with my repository in /data2/nfsmic/:
ls -lrth /data2/nfsmic/
total 68K
-rw-r--r--. 1 root root 16 Aug 15 2014 root-test-file
drwxr-xr-x. 3 root root 4.0K Mar 26 2015 mpss-3.4.3
drwxr-xr-x. 3 root root 4.0K Sep 1 2015 mpss-3.5.2
drwxr-xr-x. 3 root root 4.0K Mar 3 2016 mpss-3.6.1
drwxrwxr-x. 3 31030 31030 4.0K Mar 16 2016 ml15
drwxrwxr-x. 3 slantz slantz 4.0K Apr 14 2016 slantz
drwxrwxr-x. 4 slava77 slava77 4.0K Sep 23 2016 slava77
drwxrwxrwt. 9 root root 4.0K Aug 17 2017 scratch
drwxr-xr-x. 2 31026 31026 4.0K Jan 24 2018 CMS-Geom
drwxrwxr-x. 2 31021 31021 4.0K May 11 20:23 cerati
drwxrwxr-x. 3 31018 31018 4.0K May 15 11:26 dsr
drwxrwxr-x. 2 mkortela mkortela 4.0K Aug 7 03:19 mkortela
drwxrwxr-x. 2 31031 31031 4.0K Aug 13 09:18 mmasciov
drwxrwxr-x. 2 areinsvo areinsvo 4.0K Sep 17 07:35 areinsvo
drwxr-xr-x. 12 matevz matevz 4.0K Oct 11 17:05 matevz
drwxrwxr-x. 3 kmcdermo kmcdermo 4.0K Oct 11 21:45 kmcdermo
drwxrwxr-x. 3 mmasciov mmasciov 4.0K Oct 12 05:50 mmasciovtc

I have created a new repository (mmasciovtc), and now things appear to be working.
I don't know who the owner of the mmasciov dir is (31031), but it seems to be messed up.

kmcdermo · 2018-10-12T13:58:37Z

Ah , yes after the update to phi1 , usernames in the data directories got all screw up. @osschar, can you fix Mario's directories on phi1 and phi2?

srlantz · 2018-10-12T14:40:54Z

I took care of this. Actually while I was at it, I went ahead and assigned the correct owners and groups to all the remaining goofed-up directories (and their contents recursively) in /data2/nfsmic. (By the way, the latter directory is not nfs-mounted at this point, now that mic0 and mic1 are gone.) The exception is CMS-Geom, because I don't know who is supposed to own that one. Steve From: Kevin McDermott [mailto:notifications@github.com] Sent: Friday, October 12, 2018 9:59 AM To: cerati/mictest Cc: Subscribed Subject: Re: [cerati/mictest] Candidate ranking à la CMSSW (#167) Ah , yes after the update to phi1 , usernames in the data directories got all screw up. @osschar<https://github.com/osschar>, can you fix Mario's directories on phi1 and phi2? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#167 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AHooysmKPpDCcSG6qfYR2ly3HnHxU1mHks5ukKAPgaJpZM4XES1s>.

kmcdermo · 2018-10-12T15:06:11Z

thanks Steve!

kmcdermo · 2018-10-12T15:17:10Z

As discussed in today's meeting, we decided that this is a definite need, so we merged it. However, two points remain:

What to do about PR Candidate ranking à la CMSSW, modified to maximize efficiency and minimize fake rate #173: is there a better way to decouple the score of two candidates based on pT / eta? @mmasciov said he will look into some checks.
@osschar mentioned that the current sorting is quite heavy -- done once per candidate: would be better to store the score per candidate, then sort on that (with some extra indices). No volunteers as of yet.

cerati · 2018-10-12T15:20:34Z

Having the score per candidate will also solve the coupling, right?

…

________________________________________ From: Kevin McDermott <notifications@github.com> Sent: Friday, October 12, 2018 10:17:20 AM To: cerati/mictest Cc: Subscribed Subject: Re: [cerati/mictest] Candidate ranking à la CMSSW (#167) As discussed in today's meeting, we decided that this is a definite need, so we merged it. However, two points remain: 1. What to do about PR #173<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_cerati_mictest_pull_173&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=cZ1DN6XgZbqMf23e3rFZ6w&m=SGYGpHvR8JY5nctHTxR1pHphJ18zuLVoGIpUbbD32ds&s=cY3JiIwC4DIU_PYcq9ynIRw0m8MYd71MpDKyT7y1xlE&e=>: is there a better way to decouple the score of two candidates based on pT / eta? @mmasciov<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_mmasciov&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=cZ1DN6XgZbqMf23e3rFZ6w&m=SGYGpHvR8JY5nctHTxR1pHphJ18zuLVoGIpUbbD32ds&s=Casd35S8q8QuxJ75L-ojdIVnFg3SRpmxi4NFocQW3Co&e=> said he will look into some checks. 2. @osschar<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_osschar&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=cZ1DN6XgZbqMf23e3rFZ6w&m=SGYGpHvR8JY5nctHTxR1pHphJ18zuLVoGIpUbbD32ds&s=kMDLWrnkvfI3QtwA63TVYRaHi1Sr5w-a_cB_yRpX8ZE&e=> mentioned that the current sorting is quite heavy -- done once per candidate: would be better to store the score per candidate, then sort on that (with some extra indices). No volunteers as of yet. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_cerati_mictest_pull_167-23issuecomment-2D429361433&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=cZ1DN6XgZbqMf23e3rFZ6w&m=SGYGpHvR8JY5nctHTxR1pHphJ18zuLVoGIpUbbD32ds&s=qZFRWXJxyZa8hdES6LXpfoi6EQA2LXwDU07JcttJQZA&e=>, or mute the thread<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AEmGGouwVUNkp0QPGT83xWT-5F1fVuS0rlks5ukLKAgaJpZM4XES1s&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=cZ1DN6XgZbqMf23e3rFZ6w&m=SGYGpHvR8JY5nctHTxR1pHphJ18zuLVoGIpUbbD32ds&s=Pf7k8yWDWSIRqbyMnlJsWrPyFP1iWJeFJ2qSksn-no4&e=>.

mmasciov · 2018-10-12T15:29:15Z

@cerati Yes, but first I'll need to find a good "compromise" for PR #173 , as currently each candidate in PR #173 can have multiple scores, depending on the "other" candidate.
So, I'd first proceed "fixing" PR #173 , then use @osschar 's suggestion on top.

cerati · 2018-10-12T16:02:09Z

I thought that when doing it per candidate you would simply use the same logic avoiding taking the average (in this case the score would have a validity for different eta and pt regions). But if you really need something 'in common' between two candidates, that's the seed... but I am not sure if its parameters are available at this stage.

…

________________________________________ From: Mario Masciovecchio <notifications@github.com> Sent: Friday, October 12, 2018 10:29:16 AM To: cerati/mictest Cc: Giuseppe B. Cerati; Mention Subject: Re: [cerati/mictest] Candidate ranking à la CMSSW (#167) @cerati<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_cerati&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=cZ1DN6XgZbqMf23e3rFZ6w&m=QqpN1qzxhZIU1jyo0gjhfwDtR8nZfaXrKF-fs9L4H2M&s=8_QVANyBbNz0zk07vcz660uDanS_-O76lTc4Fz6WH3s&e=> Yes, but first I'll need to find a good "compromise" for PR #173<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_cerati_mictest_pull_173&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=cZ1DN6XgZbqMf23e3rFZ6w&m=QqpN1qzxhZIU1jyo0gjhfwDtR8nZfaXrKF-fs9L4H2M&s=0U5EkhAa6RiyxOh_DotUPdnjfOx1JSCpZlbsoJMX5kU&e=> , as currently each candidate in PR #173<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_cerati_mictest_pull_173&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=cZ1DN6XgZbqMf23e3rFZ6w&m=QqpN1qzxhZIU1jyo0gjhfwDtR8nZfaXrKF-fs9L4H2M&s=0U5EkhAa6RiyxOh_DotUPdnjfOx1JSCpZlbsoJMX5kU&e=> can have multiple scores, depending on the "other" candidate. So, I'd first proceed "fixing" PR #173<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_cerati_mictest_pull_173&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=cZ1DN6XgZbqMf23e3rFZ6w&m=QqpN1qzxhZIU1jyo0gjhfwDtR8nZfaXrKF-fs9L4H2M&s=0U5EkhAa6RiyxOh_DotUPdnjfOx1JSCpZlbsoJMX5kU&e=> , then use @osschar<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_osschar&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=cZ1DN6XgZbqMf23e3rFZ6w&m=QqpN1qzxhZIU1jyo0gjhfwDtR8nZfaXrKF-fs9L4H2M&s=RTD412VWTSNY80UIS5V1JCGnITrQGGhnxeSjJd2FwiA&e=> 's suggestion on top. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_cerati_mictest_pull_167-23issuecomment-2D429365396&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=cZ1DN6XgZbqMf23e3rFZ6w&m=QqpN1qzxhZIU1jyo0gjhfwDtR8nZfaXrKF-fs9L4H2M&s=lY4yP8OlrQ05rSlKNYAKMKuziLxpL2hs_hf6MWB6tMI&e=>, or mute the thread<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AEmGGk-5FON8og-5FPRZIK6uavoaEG6YdUWZks5ukLVMgaJpZM4XES1s&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=cZ1DN6XgZbqMf23e3rFZ6w&m=QqpN1qzxhZIU1jyo0gjhfwDtR8nZfaXrKF-fs9L4H2M&s=rYcKHJKySMhFNyoK7E7o1GiYdUZ6C0TmUqKil_e_DJI&e=>.

mmasciov added 3 commits September 20, 2018 05:44

cmsswranking

d11e4e9

Fixing indentation

f4741d6

Avoiding useless multiplications in initializations

471360e

makortel reviewed Oct 2, 2018

View reviewed changes

mmasciov added 2 commits October 11, 2018 11:15

Making code less duplicated

c801ec8

Cleaning up commented out code

dafe776

makortel reviewed Oct 11, 2018

View reviewed changes

mmasciov added 3 commits October 11, 2018 11:39

Extra cleaning of duplication for sortByScoreCandPair

a99add8

Cleaning commented out lines

0aff1c1

Removing space

7ef18e7

mmasciov mentioned this pull request Oct 11, 2018

Candidate ranking à la CMSSW, modified to maximize efficiency and minimize fake rate #173

Closed

kmcdermo merged commit a535d3a into trackreco:devel Oct 12, 2018

kmcdermo mentioned this pull request Oct 15, 2018

Add initial documentation for CMSSW integration #175

Merged

mmasciov mentioned this pull request Oct 31, 2018

Modified CMSSW ranking (seed-based, no conflict) #182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Candidate ranking à la CMSSW #167

Candidate ranking à la CMSSW #167

mmasciov commented Oct 2, 2018

makortel Oct 2, 2018

kmcdermo Oct 8, 2018

mmasciov Oct 11, 2018

makortel Oct 2, 2018

kmcdermo Oct 3, 2018

mmasciov Oct 11, 2018

kmcdermo commented Oct 8, 2018

mmasciov commented Oct 11, 2018

makortel Oct 11, 2018

mmasciov Oct 11, 2018

kmcdermo commented Oct 11, 2018

kmcdermo commented Oct 11, 2018

mmasciov commented Oct 11, 2018

kmcdermo commented Oct 11, 2018

mmasciov commented Oct 12, 2018

mmasciov commented Oct 12, 2018

kmcdermo commented Oct 12, 2018

srlantz commented Oct 12, 2018 via email

kmcdermo commented Oct 12, 2018

kmcdermo commented Oct 12, 2018

cerati commented Oct 12, 2018 via email

mmasciov commented Oct 12, 2018

cerati commented Oct 12, 2018 via email

Candidate ranking à la CMSSW #167

Candidate ranking à la CMSSW #167

Conversation

mmasciov commented Oct 2, 2018

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

kmcdermo commented Oct 8, 2018

mmasciov commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmcdermo commented Oct 11, 2018

kmcdermo commented Oct 11, 2018

mmasciov commented Oct 11, 2018

kmcdermo commented Oct 11, 2018

mmasciov commented Oct 12, 2018

mmasciov commented Oct 12, 2018

kmcdermo commented Oct 12, 2018

srlantz commented Oct 12, 2018 via email

kmcdermo commented Oct 12, 2018

kmcdermo commented Oct 12, 2018

cerati commented Oct 12, 2018 via email

mmasciov commented Oct 12, 2018

cerati commented Oct 12, 2018 via email