Phase1 Pixel DQM PixelMaps, new TrackEfficiencies, removed Harvesting, new Plots and various Bugfixes #17208
Conversation
Does not really add new features, but is certainly the better way to do things.
We cannot automatically determine that, but need to pass it to some Pixel Tools.
Conflicts: DQM/SiPixelPhase1Common/python/HistogramManager_cfi.py
There were columns removed from the COUNT step, which caused missing counters, and the empty dummy column only causing trouble now.
No more harvesting needed. Also cleans up the plot layouts a bit.
It proved to be not very useful and fairly esoteric to use. With the TarackEfficiency plugin it lost its most important use case; the TProfile based efficiencies are simpler and closer in philosophy to the Phase1 DQM framework.
…sing SiPixelCoordinates (new class) cherry-pick: removed references to ntuplizer code for CMSSW/DQM, moved files. Conflicts: plugins/BuildFile.xml plugins/PhaseIPixelNtuplizer.cc plugins/PhaseIPixelNtuplizer.h test/run_PhaseIPixelNtuplizer_MinBias_cfg.py
Also first experiments on Pixel Maps.
Style might still need some tuning (use different functions from SiPixelCoordinates), some more quantities could use it, and there might be some off-by-1 or off-by-0.5's for the row/cols.
schneiml
force-pushed
the
phase1pixeldqm-post-christmas-version
branch
from
8f92a87
to
45acd04
Compare
|
@dmitrijus I removed the last commit, seems to cause more trouble than expected. Can you make sure this gets re-tested? |
|
please test |
cmsbuild
added
comparison-pending
tests-pending
and removed
comparison-available
tests-started
labels
|
The tests are being triggered in jenkins. |
|
Comparison job queued. |
schneiml
changed the title
| @@ -0,0 +1,916 @@ | |||
| // -*- C++ -*- | |||
There was a problem hiding this comment.
hi @schneiml - dqm looks like the wrong spot for such code. Did you consider other users of this?
Second, did you do some study to justify the caching layer? Given the simplicity of the calculations I would think that searching a big map is both slower and more memory intensive than just redoing the calculation.
There was a problem hiding this comment.
Re where to put this: Originally, this file is by @jkarancs and lives here: https://github.com/jkarancs/PhaseIPixelNtuplizer/blob/31e12769af7eb82dd7c1f1c4418641e4d2fe8b7d/src/SiPixelCoordinates.cc
There were some plans to put this into a DPGAnalysis package (to be created), but I think having DQM depend on that is not good either. So, we are looking for a better place, but to have it available now I just threw it into the DQM package. If you have better suggestions (this is not Phase1, but Pixel specific; it is for now only used in DQM and that will probably stay like that; but external analysis code will also use it) please let us know.
Re the caching: Yes, this is probably too aggressive. There are some really slow operations in there (around channels/ROCs), but in many cases avoiding it might be better. From what I have measured, this class causes ~10% of the online DQM cost (which is basically only Pixel DQM), in usual offline sequences the Pixel DQM is only a few percent of the total. So there is an opportunity to improve a bit, but it seems not too urgent to me and I wanted to keep @jkarancs's state for now (up to a bugfix).
There was a problem hiding this comment.
thanks
Sounds like it belongs in a data format (tracker already has much of this sort of functionality in a data format) or in a reco package.
what is the impact on memory of this cache?
There was a problem hiding this comment.
Re memory: I have not measured here, but there are "only" ~1900 Pixel modules in Phase1, so each of the Maps should stay in the range of Kilobytes. There will be one instance of this per Phase1 DQM plugin (and also per thread), so in the end it might be in the lower megabytes.
Re the location: Just to make it clear, this file will not move unless a concrete new path is proposed. I'd happily move it if somebody tells me where to move it...
There was a problem hiding this comment.
My proposal is to move it to DataFormats/SiPixelDetId.
I think this is what David had in mind.
There was a problem hiding this comment.
Update: I fail to get SiPixelCoordinates compiled in the DataFormats/SiPixelDetId package:
/cvmfs/cms-ib.cern.ch/week0/slc6_amd64_gcc530/cms/cmssw-patch/CMSSW_9_0_X_2017-01-24-2300/src/DataFormats/CLHEP/interface/AlgebraicObjects.h:8:33: fatal error: CLHEP/Matrix/Vector.h: No such file or directory
Which is transitively included from SiPixelCoordinates, but worked in the DQM package. I do't see what is going on there...
There was a problem hiding this comment.
If it's a compilation issue, my experience is that you need to add something to the BuildFile:
https://github.com/cms-sw/cmssw/blob/CMSSW_9_0_X/DataFormats/SiPixelDetId/BuildFile.xml
Compare what is in the DQM package that is missing in the DataFormats:
https://github.com/cms-sw/cmssw/blob/CMSSW_9_0_X/DQM/SiPixelPhase1Common/BuildFile.xml
There was a problem hiding this comment.
@schneiml - you need:
<use name="clhep"/>
There was a problem hiding this comment.
@jkarancs looked at it, but I don't see any obvious candidate...
@ianna Wow, thanks! I guess this was loaded as a dependency of sth. in the DQM package.
Now just lots of linking errors, but there it is more clear what needs to be added...
Update: Now lets see if this can be satisfied w/o dependency cycles...
There was a problem hiding this comment.
@jkarancs @davidlange6 it seems DataFormats/SiPixelDetId is not a feasible location since it would cause a recursive dependency via Geometry/TrackerGeometryBuilder.
In general, a package with the SiPixelCoordinatesin it would have quite massive dependencies. Which is not that big of a problem with the DQM package, since it has those dependencies either way...
|
@davidlange6 any specific plans on this? I will loose |
|
Hi @schneiml - sorry for the slow answer - i had missed the geometry dependency. likely some restructuring is needed - but for now i guess DQM is as good of a home as any.. |
|
+1 |
Changes from #17089 plus more:
Should go into pre3.
@fioriNTU @enochnotsocool @davidcarbonis