Cluster charge cut, take two #162
Conversation
|
Do we want to have a place where the negative hit indices are documented? I am not suggesting to delay this PR, just bringing up the issue. |
|
@areinsvo thanks! This looks much better :). Feel free to close #161. From a review standpoint: this looks ready to go, although as already mentioned, lets wait until #154 and #160 are merged, rebase again (such fun), and then run the validation. @cerati I think this is a fair point. Thankfully, with #160, we now have a pretty thorough documentation of the code, and I think we can add it to the list of things that need to be added after this PR and #160 are merged. I will open an issue on what still needs to go into the README. Documenting the hit index meanings can also go in another .txt file, in the vein of cmssw-trackerinfo-desc.txt and validation-desc.txt, appending the list of resources in Section 8 of the README to point to this new file. |
|
We need to remember to update the memory files when we merge this PR (otherwise it will have no effect...) |
For sure, this is a few line change in: @slava77 Do we have the memory files with this cut for all the different samples we use? We also need to copy them to the disks of phi1 and phi2 for benchmarking. Or as @osschar , we could always alias just one set, and have the different platforms read across different machines... although this would convolute in I/O as well (especially since the compute tests are concurrent on different platforms). samples (off the top of my head)
|
I didn't try to remake the files yet. |
|
@slava77 do you have a script to generate all of the necessary memory files? If not, could you specify which flags are used for the different naming conventions? I'll probably add a string to the name to indicate which value of the CCC was used. It doesn't matter to me which of us makes them. I already have some of the files, but I would probably regenerate them anyway to be 100% sure the appropriate flags are set. |
|
On 8/31/18 9:21 AM, areinsvo wrote:
@slava77 <https://github.com/slava77> do you have a script to generate
all of the necessary memory files? If not, could you specify which flags
are used for the different naming conventions? I'll probably add a
string to the name to indicate which value of the CCC was used.
It doesn't matter to me which of us makes them. I already have some of
the files, but I would probably regenerate them anyway to be 100% sure
the appropriate flags are set.
I produced two kinds of files in the past
1. saving only sim tracks with at least N (3?) hits
./writeMemoryFile --input $fIn --output $fOut --write-rec-tracks
--clean-sim-tracks --write-all-events
this is what .clean. stands for in the file names
2. files with all sim tracks from the tracking ntuple
this does not have --clean-sim-tracks
The one-liner to process everything for the last round was:
find /pathToTrackingNtuples/ -name trackingNtuple.root | while read -r
fIn; do fOut=`echo $fIn | sed -e
's/trackingNtuple.root/memoryFile.fv3.clean.writeAll.recT.081318-4551fbf.bin/g'`;
[ -f "$fOut" ] && echo "$fOut exists: skipping" && continue; echo $fIn
$fOut > ${fOut}.log; date >> ${fOut}.log; ./writeMemoryFile --input $fIn
--output $fOut --write-rec-tracks --clean-sim-tracks --write-all-events
--verbosity 2 | grep -v "^SKIP\|^lay=\|^track #" >> ${fOut}.log 2>&1; done
|
|
@slava77 apologies for not getting to this earlier in the week. Thanks for sending the command to generate the memory files. I don't have permission to write in the directories within /data2/slava77/samples/2017/pass-c93773a/initialStep/ but the files can be found at /home/users/areinsvo/memoryFiles. Would you be able to move them to your area so everything is together? I generated four memory files for each sample: with and without clean tracks x with and without the CCC. As @kmcdermo pointed out, these might need to be copied to phi1 and phi2 as well. |
tkNtuple/WriteMemoryFile.cc
Outdated
| { | ||
| applyCCC = true; | ||
| if( next_arg_option(mArgs, i)) | ||
| { |
There was a problem hiding this comment.
the indentation looks off
tkNtuple/WriteMemoryFile.cc
Outdated
| layerHits_[ilay].push_back(hit); | ||
| MCHitInfo hitInfo(simTkIdx, ilay, layerHits_[ilay].size()-1, totHits); | ||
| simHitsInfo_.push_back(hitInfo); | ||
| totHits++; |
There was a problem hiding this comment.
please indent the block inside the if
There was a problem hiding this comment.
@slava77 I fixed the first indentation, but in lines 950 to 953 the block is already indented, as far as I can tell.
There was a problem hiding this comment.
that's not what I see in the browser.
It seems to be a tab vs space issue.
|
It looked indented to me in the browser, but I made another commit where the indentation should be more obvious. |
thank you. |
|
I finally finished redistributing the files
perhaps someone with sudo could In each directory there is a
@areinsvo thank you for making these. |
|
I tried running the benchmark scripts on the new memory files, but I ran into an issue with the validation. With @kmcdermo's help I narrowed down the problem, but it hasn't been solved yet. The summary of the problem is below:
For now, the plots I have (everything except the SIMVAL plots) can be found at http://areinsvo.web.cern.ch/areinsvo/pull162/ |
|
The SIMVAL plots for events in flight = 1 can now be found at http://areinsvo.web.cern.ch/areinsvo/pull162/SIMVAL_NoMEIF/SIMVAL/ Also, I committed changes to the benchmark and validation scripts to point to the new memory files (thanks @slava77 for copying those), and I updated the documentation slightly to clarify a few minor issues I had setting up my website for plots. |
* deregister context of TFile to allow writing from multiple threads
* make TFile's and TTree's std::unique_ptr
* set directory of TTree to 0 when initialized, then set to file at the end
Fix MEIF with ROOT validation + CCC cut
|
@kmcdermo fixed the issues with MEIF and found one issue with the CCC. His changes have been merged with my branch. The full set of validation plots after his changes can be found here: If you want to compare these to the validation plots for the new memory file without the cluster charge cut, those can be found here: As expected, the CMSSW performance lines are exactly the same between the two memory files. The weird behavior we saw at the end of last week must have been due to the bugs that Kevin fixed. |
|
Ah, this is great! Okay, I will merge this now. One point to mention: we noticed that the simtrack validation changed slightly between the two versions of the old and new memory files. Compare:
As Allie already stated the cmssw track efficiency are the same in 2. and 3. (as they should be!), and the mkFit tracks move up in efficiency. This same effect was already seen with the old memory files, applying the CCC by hand. However, both mkFit and cmssw tracks change w.r.t. to 1., although this looks mostly statistical. Perhaps different events/tracks were written to the memory file? Just looking at the top lines of the text file dumps, it is clear different tracks and/or events were saved:
So, I think this can be safely merged, although it would be great to understand why the memory file would write different tracks/events. |
|
On 9/17/18 9:56 AM, Kevin McDermott wrote:
However, both mkFit and cmssw tracks change w.r.t. to 1., although this
looks mostly statistical. Perhaps different events/tracks were written
to the memory file? Just looking at the top lines of the text file
dumps, it is clear different tracks and/or events were saved:
yes, the ttbar PU70HS sample was remade: the minbias events are not the
same.
|
|
ah, okay, thanks for the clarification! |
The commits got messed up in PR161 The code on this branch is unchanged with respect to the previous pull request, but here the history should be fixed. Thanks @kmcdermo for your help.
This PR includes the implementation of the cluster charge cut (CCC) that I have been working on and presented at the last two group meetings. The default value of the cut is 1620, and the --apply-ccc flag has to be called when running writeMemoryFile in order for the cut to be applied. The flag --apply-ccc can also take an argument to change the value of the cut.
In addition, there is a minor change to tkNtuple/Makefile to properly include ../lib in the path. I've also included Mario's script to compare efficiencies, duplicate rates, and fake rates before and after changes. This can now be found in the plotting directory.
Merging this pull request should wait until the changes to the benchmarking procedure are merged and after phiphi is available for testing.