Flag merged clusters for relaxed CPE errors #6349
Conversation
|
A new Pull Request was created by @wmtford for CMSSW_7_3_X. Flag merged clusters for relaxed CPE errors It involves the following packages: DataFormats/SiStripCluster @cmsbuild, @nclopezo, @StoyanStoynev, @slava77 can you please review it and eventually sign? Thanks. |
|
-1 runTheMatrix-results/4.53_RunPhoton2012B+RunPhoton2012B+HLTD+RECODreHLT+HARVESTDreHLT/step3_RunPhoton2012B+RunPhoton2012B+HLTD+RECODreHLT+HARVESTDreHLT.log ----- Begin Fatal Exception 12-Nov-2014 12:55:39 CET----------------------- An exception of category 'Configuration' occurred while [0] Constructing the EventProcessor [1] Constructing module: class=SiStripClusterizer label='calZeroBiasClusters' Exception Message: MissingParameter: Parameter 'doRefineCluster' not found. ----- End Fatal Exception ------------------------------------------------- 1001.0 step2 runTheMatrix-results/1001.0_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD/step2_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD.log ----- Begin Fatal Exception 12-Nov-2014 12:56:55 CET----------------------- An exception of category 'Configuration' occurred while [0] Constructing the EventProcessor [1] Constructing module: class=SiStripClusterizer label='calZeroBiasClusters' Exception Message: MissingParameter: Parameter 'doRefineCluster' not found. ----- End Fatal Exception ------------------------------------------------- you can see the results of the tests here: |
| edm::ESHandle<SiStripQuality> quality = 0; | ||
| SiStripDetInfoFileReader* reader = 0; | ||
| if (doRefineCluster_) { | ||
| es.get<SiStripQualityRcd>().get("", quality); |
There was a problem hiding this comment.
@slava77 as I stated in the previous pull request, I believe this to be an illegal use of a Service since it can modify the physics results.
There was a problem hiding this comment.
I think that combining properly the information from the cabling (ES object) and from SiStripQuality (ES object), it should be possible to achieve what is now done with SiStripDetInfoFileReader and SiStripQuality. But I acknowledge that it may require some quiet time to be sure that all the cases are handled correctly.
Andrea
On Nov 12, 2014, at 14:19 , Chris Jones <notifications@github.commailto:notifications@github.com>
wrote:
In RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cc:
@@ -24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es) {
algorithm->initialize(es);
- edm::ESHandle quality = 0;
- SiStripDetInfoFileReader* reader = 0;
- if (doRefineCluster_) {
- es.get().get("", quality);
@slava77https://github.com/slava77 as I stated in the previous pull request, I believe this to be an illegal use of a Service since it can modify the physics results.
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6349/files#r20217700.
There was a problem hiding this comment.
Hi Andrea,
It appears to me that to getting the total number of strips in a sensor
could be accomplished along the lines sketched here:
In SiStripClusterizer.h
#include "CalibFormats/SiStripObjects/interface/SiStripDetCabling.h"
#include "CalibTracker/Records/interface/SiStripDetCablingRcd.h"
void beginRun(const edm::Run& run, const edm::EventSetup& es ) override;
Declare a map(detId, Nstrips) NstripMap_;
In the .cc file
void SiStripClusterizer::beginRun(const edm::Run& run, const
edm::EventSetup& es ) {
edm::ESHandle SiStripDetCabling;
es.get().get( SiStripDetCabling);
Loop over strip detIds {
int Nstrips = SiStripDetCabling.nAPvPairs(detId) * 2 * 128;
fill NstripMap_
}
}
(I suppose this could be under ESWatcher if it's worth while.)
Then in the existing refineCluster function we just have
int nchannideal = NstripMap(detId);
Do you see any gotchas with this?
(I'm assuming that EDProducer has a BeginRun().)
Bill
On 11/12/14 6:26 AM, venturia wrote:
In RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cc:
@@ -24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es) {
algorithm->initialize(es);
- edm::ESHandle quality = 0;
- SiStripDetInfoFileReader* reader = 0;
- if (doRefineCluster_) {
- es.get().get("", quality);
I think that combining properly the information from the cabling (ES
object) and from SiStripQuality (ES object), it should be possible to
achieve what is now done with SiStripDetInfoFileReader and
SiStripQuality. But I acknowledge that it may require some quiet time to
be sure that all the cases are handled correctly. Andrea On Nov 12,
2014, at 14:19 , Chris Jones
<notifications@github.commailto:notifications@github.com> wrote: In
RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cc:
@@ -24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es)
{ algorithm->initialize(es); + edm::ESHandle quality =
0; + SiStripDetInfoFileReader* reader = 0; + if (doRefineCluster_) { +
es.get().get("", quality);
@slava77 https://github.com/slava77https://github.com/slava77 as I
stated in the previous pull request, I believe this to be an illegal use
of a Service since it can modify the physics results. — Reply to this
email directly or view it on
GitHubhttps://github.com//pull/6349/files#r20217700.—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/6349/files#r20217984.
There was a problem hiding this comment.
Hi Bill,
I am not 100% sure it can work in that way. Actually I am not 100% sure I am right saying that it does not work.
As usual the problem is for those modules which miss the middle (or the first) APV pair. In that case the number of APVs is less than what it should be, and that is fine, but it does not mean that the strip number has to stop at 128*nAPVs .
My suggestion is to look for a few modules with these features and check the algorithm (comparing the output with what you would get with your original algorithm which is fine but illegal).
I don't know exactly which data you are using to test your code but can look at the summary reports of the cabling content for each IOV where the list of all the (partly) bad modules is printed. You can find them here for the cabling tag which is used since "ever":
https://test-stripdbmonitor.web.cern.ch/test-stripdbmonitor/CondDBMonitoring/cms_orcoff_prod/CMS_COND_31X_STRIP/DBTagCollection/SiStripFedCabling/SiStripFedCabling_GR10_v1_hlt/CablingLog/
Look at the files named QualityInfoFromCabling_Run…txt choosing the IOV you are interested in and look at the detailed list of modules at the end. By selecting those with "Fibers" patterns like 100 or 010 or 110 you will select modules with the first, the second or the first and the second fibers not working. For example 369137741, 402674837, 402675376, 402676914, 402664582, 436244845, 436245994
…
Incidentally, if/when we find a solution it could be useful to implement a method in SiStripQuality which returns this piece of information and use it everywhere, presently, we are using SiStripFileReader to achieve that. I am also wondering if we have to modify SiStripQuality to return a bad boolean when asked for a strip out of range.
If instead we do not find a sensible solution we have to think about a way to move the information in SiStripFileReader into an ES object in the DB.
Andrea
,
On Nov 14, 2014, at 17:27 , wmtford <notifications@github.commailto:notifications@github.com> wrote:
In RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cc:
@@ -24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es) {
algorithm->initialize(es);
- edm::ESHandle quality = 0;
- SiStripDetInfoFileReader* reader = 0;
- if (doRefineCluster_) {
- es.get().get("", quality);
Hi Andrea, It appears to me that to getting the total number of strips in a sensor could be accomplished along the lines sketched here: In SiStripClusterizer.h #include "CalibFormats/SiStripObjects/interface/SiStripDetCabling.h" #include "CalibTracker/Records/interface/SiStripDetCablingRcd.h" void beginRun(const edm::Run& run, const edm::EventSetup& es ) override; Declare a map(detId, Nstrips) NstripMap_; In the .cc file void SiStripClusterizer::beginRun(const edm::Run& run, const edm::EventSetup& es ) { edm::ESHandle SiStripDetCabling; es.get().get( SiStripDetCabling); Loop over strip detIds { int Nstrips = SiStripDetCabling.nAPvPairs(detId) * 2 * 128; fill NstripMap_ } } (I suppose this could be under ESWatcher if it's worth while.) Then in the existing refineCluster function we just have int nchannideal = NstripMap(detId); Do you see any gotchas with this? (I'm assuming that EDProducer has a BeginRun().) Bill
…x-msg://157/#
On 11/12/14 6:26 AM, venturia wrote: In RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cc: > @@ -24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es) { > > algorithm->initialize(es); > > + edm::ESHandle quality = 0; > + SiStripDetInfoFileReader* reader = 0; > + if (doRefineCluster_) { > + es.get().get("", quality); I think that combining properly the information from the cabling (ES object) and from SiStripQuality (ES object), it should be possible to achieve what is now done with SiStripDetInfoFileReader and SiStripQuality. But I acknowledge that it may require some quiet time to be sure that all the cases are handled correctly. Andrea On Nov 12, 2014, at 14:19 , Chris Jones <notifications@github.commailto:notifications@github.commailto:notifications@github.com> wrote: In RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cc: @@ -24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es) { algorithm->initialize(es); + edm::ESHandle quality = 0; + SiStripDetInfoFileReader* reader = 0; + if (doRefineCluster_) { + es.get().get("", quality); @slava77https://github.com/slava77 https://github.com/slava77https://github.com/slava77https://github.com/slava77%3E%3Chttps://github.com/slava77 as I stated in the previous pull request, I believe this to be an illegal use of a Service since it can modify the physics results. — Reply to this email directly or view it on GitHubhttps://github.com//pull/6349/files#r20217700. — Reply to this email directly or view it on GitHub https://github.com/cms-sw/cmssw/pull/6349/files#r20217984.
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6349/files#r20368828.
There was a problem hiding this comment.
Hi Andrea,
I'm running on Monte Carlo,
/store/relval/CMSSW_7_3_0_pre1/RelValTTbar_13/GEN-SIM/PRE_LS172_V15-v1/
Do you know if the effects of concern are modeled in these datasets?
What does IOV mean? I guess it must be a run interval or lumisection
block to which a set of these conditions pertains? So if running on
data I'd look at the first of the following files if my run numbers were
between those in the file names?
QualityInfoFromCabling_Run192835.txt QualityInfoFromCabling_Run195790.txt
Just to be sure I understand your concern,
nchannideal = reader->getNumberOfApvsAndStripLength(detId).first*128
will use the number of APVs that are nominally in the system, whereas
nchannideal = SiStripDetCabling_->nApvPairs(detId) * 2 * 128
may be using the number of live APVs successfully read out (in the most
recent calibration run), which may be fewer than the nominal. Do I have
it right?
Bill
On 11/15/14 11:38 AM, venturia wrote:
In RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cc:
@@ -24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es) {
algorithm->initialize(es);
- edm::ESHandle quality = 0;
- SiStripDetInfoFileReader* reader = 0;
- if (doRefineCluster_) {
- es.get().get("", quality);
Hi Bill, I am not 100% sure it can work in that way. Actually I am not
100% sure I am right saying that it does not work. As usual the problem
is for those modules which miss the middle (or the first) APV pair. In
that case the number of APVs is less than what it should be, and that is
fine, but it does not mean that the strip number has to stop at
128_nAPVs . My suggestion is to look for a few modules with these
features and check the algorithm (comparing the output with what you
would get with your original algorithm which is fine but illegal). I
don't know exactly which data you are using to test your code but can
look at the summary reports of the cabling content for each IOV where
the list of all the (partly) bad modules is printed. You can find them
here for the cabling tag which is used since "ever":
https://test-stripdbmonitor.web.cern.ch/test-stripdbmonitor/CondDBMonitoring/cms_orcoff_prod/CMS_COND_31X_STRIP/DBTagCollection/SiStripFedCabling/SiStripFedCabling_GR10_v1_hlt/CablingLog/
Look at the files named QualityInfoFromCabling_Run…txt choosing the IOV
you are interested in and look at the detailed list of modules at the
end. By selecting those with "Fibers" patterns like 100 or 010 or 110
you will select modules with the first, the second or the first and the
second fibers not working. For example 369137741, 402674837, 402675376,
402676914, 402664582, 436244845, 436245994 … Incidentally, if/when we
find a solution it could be useful to implement a method in
SiStripQuality which returns this piece of information and use it
everywhere, presently, we are using SiStripFileReader to achieve that. I
am also wondering if we have to modify SiStripQuality to return a bad
boolean when asked for a strip out of range. If instead we do not find a
sensible solution we have to think about a way to move the information
in SiStripFileReader into an ES object in the DB. Andrea , On Nov 14,
2014, at 17:27 , wmtford
<notifications@github.commailto:notifications@github.com> wrote: In
RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cc:
@@ -24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es)
{ algorithm->initialize(es); + edm::ESHandle quality =
0; + SiStripDetInfoFileReader_ reader = 0; + if (doRefineCluster_) { +
es.get().get("", quality);
Hi Andrea, It appears to me that to getting the total number of strips
in a sensor could be accomplished along the lines sketched here: In
SiStripClusterizer.h #include
"CalibFormats/SiStripObjects/interface/SiStripDetCabling.h" #include
"CalibTracker/Records/interface/SiStripDetCablingRcd.h" void
beginRun(const edm::Run& run, const edm::EventSetup& es ) override;
Declare a map(detId, Nstrips) NstripMap_; In the .cc file void
SiStripClusterizer::beginRun(const edm::Run& run, const edm::EventSetup&
es ) { edm::ESHandle SiStripDetCabling;
es.get().get( SiStripDetCabling); Loop over strip
detIds { int Nstrips = SiStripDetCabling.nAPvPairs(detId) * 2 * 128;
fill NstripMap_ } } (I suppose this could be under ESWatcher if it's
worth while.) Then in the existing refineCluster function we just have
int nchannideal = NstripMap(detId); Do you see any gotchas with this?
(I'm assuming that EDProducer has a BeginRun().) Bill …x-msg://157/#
On 11/12/14 6:26 AM, venturia wrote: In
RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cc: > @@
-24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es) {algorithm->initialize(es); > > + edm::ESHandle
quality = 0; > + SiStripDetInfoFileReader* reader = 0; > + if
(doRefineCluster_) { > + es.get().get("", quality); I
think that combining properly the information from the cabling (ES
object) and from SiStripQuality (ES object), it should be possible to
achieve what is now done with SiStripDetInfoFileReader and
SiStripQuality. But I acknowledge that it may require some quiet time to
be sure that all the cases are handled correctly. Andrea On Nov 12,
2014, at 14:19 , Chris Jones
<notifications@github.commailto:notifications@github.commailto:notifications@github.com>
wrote: In
RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cc: @@
-24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es) {
algorithm->initialize(es); + edm::ESHandle quality = 0;
- SiStripDetInfoFileReader* reader = 0; + if (doRefineCluster_) { +
es.get().get("", quality); @slava77
https://github.com/slava77https://github.com/slava77
https://github.com/slava77https://github.com/slava77https://github.com/slava77%3E%3Chttps://github.com/slava77
as I stated in the previous pull request, I believe this to be an
illegal use of a Service since it can modify the physics results. —
Reply to this email directly or view it on
GitHubhttps://github.com/Flag merged clusters for relaxed CPE errors #6349/files#r20217700. —
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/6349/files#r20217984. — Reply to
this email directly or view it on
GitHubhttps://github.com/Flag merged clusters for relaxed CPE errors #6349/files#r20368828.—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/6349/files#r20404056.
There was a problem hiding this comment.
Bill,
in the MC the cabling is ideal and all the bad components are in the bad channel records. In the data part of the bad components are in the cabling as “missing pieces”. Therefore you need the real data to test your algorithm.ù
IOV means interval of validity and, in case of the cabling, is the first run number starting from which the cabling is valid. Therefore you have to choose the IOV with the largest run number smaller than the run you are analyzing.
what you wrote about nApvPairs should be correct: what it may not be correct is that when you get, let’s say, one APV pair, then the correct thing to do is to loop from 0 to 255 and check for the individual strips. It may be that you have to loop from 256 to 512 because the missing pair is the first.
I don’t have a suggestion from the top of my head now and I should look at the code of the cabling object to check if there is anything you can use to overcome this. Maybe looping what are called “connections” and check if they are valid (or if the pointer is not-null) and from that understand the pattern of the missing APVs. But I do not remember if the following simple rule apply to all the modules, both 2 and 3 APV pair modules:
connections[0] : strips 1-255
connections[1] : strips 256-511
connections[2] : strips 512-767
Andrea
Il giorno 16/nov/2014, alle ore 04:02, wmtford <notifications@github.commailto:notifications@github.com> ha scritto:
In RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cchttp://SiStripClusterizer.cc:
@@ -24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es) {
algorithm->initialize(es);
- edm::ESHandle quality = 0;
- SiStripDetInfoFileReader* reader = 0;
- if (doRefineCluster_) {
- es.get().get("", quality);
Hi Andrea, I'm running on Monte Carlo, /store/relval/CMSSW_7_3_0_pre1/RelValTTbar_13/GEN-SIM/PRE_LS172_V15-v1/ Do you know if the effects of concern are modeled in these datasets? What does IOV mean? I guess it must be a run interval or lumisection block to which a set of these conditions pertains? So if running on data I'd look at the first of the following files if my run numbers were between those in the file names? QualityInfoFromCabling_Run192835.txt QualityInfoFromCabling_Run195790.txt Just to be sure I understand your concern, nchannideal = reader->getNumberOfApvsAndStripLength(detId).first_128 will use the number of APVs that are nominally in the system, whereas nchannideal = SiStripDetCabling_->nApvPairs(detId) * 2 * 128 may be using the number of live APVs successfully read out (in the most recent calibration run), which may be fewer than the nominal. Do I have it right? Bill
…x-msg://2/#
On 11/15/14 11:38 AM, venturia wrote: In RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cchttp://SiStripClusterizer.cc: > @@ -24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es) { > > algorithm->initialize(es); > > + edm::ESHandle quality = 0; > + SiStripDetInfoFileReader_ reader = 0; > + if (doRefineCluster_) { > + es.get().get("", quality); Hi Bill, I am not 100% sure it can work in that way. Actually I am not 100% sure I am right saying that it does not work. As usual the problem is for those modules which miss the middle (or the first) APV pair. In that case the number of APVs is less than what it should be, and that is fine, but it does not mean that the strip number has to stop at 128_nAPVs . My suggestion is to look for a few modules with these features and check the algorithm (comparing the output with what you would get with your original algorithm which is fine but illegal). I don't know exactly which data you are using to test your code but can look at the summary reports of the cabling content for each IOV where the list of all the (partly) bad modules is printed. You can find them here for the cabling tag which is used since "ever": https://test-stripdbmonitor.web.cern.ch/test-stripdbmonitor/CondDBMonitoring/cms_orcoff_prod/CMS_COND_31X_STRIP/DBTagCollection/SiStripFedCabling/SiStripFedCabling_GR10_v1_hlt/CablingLog/ Look at the files named QualityInfoFromCabling_Run…txt choosing the IOV you are interested in and look at the detailed list of modules at the end. By selecting those with "Fibers" patterns like 100 or 010 or 110 you will select modules with the first, the second or the first and the second fibers not working. For example 369137741, 402674837, 402675376, 402676914, 402664582, 436244845, 436245994 … Incidentally, if/when we find a solution it could be useful to implement a method in SiStripQuality which returns this piece of information and use it everywhere, presently, we are using SiStripFileReader to achieve that. I am also wondering if we have to modify SiStripQuality to return a bad boolean when asked for a strip out of range. If instead we do not find a sensible solution we have to think about a way to move the information in SiStripFileReader into an ES object in the DB. Andrea , On Nov 14, 2014, at 17:27 , wmtford <notifications@github.commailto:notifications@github.commailto:notifications@github.com> wrote: In RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cchttp://SiStripClusterizer.cc: @@ -24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es) { algorithm->initialize(es); + edm::ESHandle quality = 0; + SiStripDetInfoFileReader_ reader = 0; + if (doRefineCluster_) { + es.get().get("", quality); Hi Andrea, It appears to me that to getting the total number of strips in a sensor could be accomplished along the lines sketched here: In SiStripClusterizer.h #include "CalibFormats/SiStripObjects/interface/SiStripDetCabling.h" #include "CalibTracker/Records/interface/SiStripDetCablingRcd.h" void beginRun(const edm::Run& run, const edm::EventSetup& es ) override; Declare a map(detId, Nstrips) NstripMap_; In the .cc file void SiStripClusterizer::beginRun(const edm::Run& run, const edm::EventSetup& es ) { edm::ESHandle SiStripDetCabling; es.get().get( SiStripDetCabling); Loop over strip detIds { int Nstrips = SiStripDetCabling.nAPvPairs(detId) * 2 * 128; fill NstripMap_ } } (I suppose this could be under ESWatcher if it's worth while.) Then in the existing refineCluster function we just have int nchannideal = NstripMap(detId); Do you see any gotchas with this? (I'm assuming that EDProducer has a BeginRun().) Bill …x-msg://157/# On 11/12/14 6:26 AM, venturia wrote: In RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cchttp://SiStripClusterizer.cc: > @@ -24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es) { > > algorithm->initialize(es); > > + edm::ESHandle quality = 0; > + SiStripDetInfoFileReader* reader = 0; > + if (doRefineCluster_) { > + es.get().get("", quality); I think that combining properly the information from the cabling (ES object) and from SiStripQuality (ES object), it should be possible to achieve what is now done with SiStripDetInfoFileReader and SiStripQuality. But I acknowledge that it may require some quiet time to be sure that all the cases are handled correctly. Andrea On Nov 12, 2014, at 14:19 , Chris Jones <notifications@github.commailto:notifications@github.commailto:notifications@github.commailto:notifications@github.com> wrote: In RecoLocalTracker/SiStripClusterizer/plugins/SiStripClusterizer.cchttp://SiStripClusterizer.cc: @@ -24,8 +31,18 @@ produce(edm::Event& event, const edm::EventSetup& es) { algorithm->initialize(es); + edm::ESHandle quality = 0; + SiStripDetInfoFileReader* reader = 0; + if (doRefineCluster_) { + es.get().get("", quality); @slava77https://github.com/slava77 https://github.com/slava77https://github.com/slava77https://github.com/slava77%3E%3Chttps://github.com/slava77 https://github.com/slava77https://github.com/slava77<https://github.com/slava77%3E%3Chttps://github.com/slava77https://github.com/slava77%3E%3Chttps://github.com/slava77%3Chttps://github.com/slava77%3E%3Chttps://github.com/slava77> as I stated in the previous pull request, I believe this to be an illegal use of a Service since it can modify the physics results. — Reply to this email directly or view it on GitHubhttps://github.com//pull/6349/files#r20217700. — Reply to this email directly or view it on GitHub https://github.com/cms-sw/cmssw/pull/6349/files#r20217984. — Reply to this email directly or view it on GitHubhttps://github.com//pull/6349/files#r20368828. — Reply to this email directly or view it on GitHub https://github.com/cms-sw/cmssw/pull/6349/files#r20404056.
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/6349/files#r20406815.
| @@ -33,7 +33,7 @@ class SiStripCluster { | |||
|
|
|||
| /** The number of the first strip in the cluster | |||
| */ | |||
| uint16_t firstStrip() const {return firstStrip_;} | |||
| uint16_t firstStrip() const {return firstStrip_ & 0x7FFF;} | |||
There was a problem hiding this comment.
I'm sorry for this, but is it possible to add a comment which explains a little bit what stands for ?
thanks much
There was a problem hiding this comment.
Hi Mia,
Sure. I've just made the following edits in my working directory. Let
me know if you think it's clear. I decided that one could conceivably
want to set the merged bit via the constructor as well:
template
SiStripCluster(const uint16_t& firstStrip, Iter begin, Iter end, bool
merged):
amplitudes_(begin,end), firstStrip_(firstStrip), error_x(-99999.9) {
if (merged) firstStrip_ |= 0x8000; // if this is a candidate merged
cluster
}
/** The number of the first strip in the cluster
* The high bit of firstStrip_ is set by setMerged() to indicate that
* the cluster is a candidate for being merged.
*/
uint16_t firstStrip() const {return firstStrip_ & 0x7FFF;}
/** Test (set) the merged status of the cluster
*
*/
bool isMerged() const {return (firstStrip_ & 0x8000) != 0;}
void setMerged(bool mergedState) {mergedState ? firstStrip_ |= 0x8000
: firstStrip_ &= 0x7FFF;}
// strip centers are offcet by half pitch w.r.t. strip numbers,
// so one has to add 0.5 to get the correct barycenter position.
// Need to mask off the high bit of firstStrip_, which contains the
merged status.
return float((firstStrip_ & 0x7FFF)) + float(sumx) / float(suma) + 0.5f;
Bill
On 11/12/2014 10:19 AM, mia tosi wrote:
In DataFormats/SiStripCluster/interface/SiStripCluster.h:
@@ -33,7 +33,7 @@ class SiStripCluster {
/** The number of the first strip in the cluster
*/
- uint16_t firstStrip() const {return firstStrip_;}
- uint16_t firstStrip() const {return firstStrip_ & 0x7FFF;}
I'm sorry for this, but is it possible to add a comment which explains a
little bit what stands for ?
thanks much—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/6349/files#r20234011.
There was a problem hiding this comment.
thanks much,
but I'm sorry (I'm a little bit ignorant on this)
what do they mean
& 0x7FFF ?
& 0x8000 ?
|= 0x8000 ?
&= 0x7FFF
I understand they are operator on binary objects (which are the detIDs, I think)
but it is not that straightforward to me the operation :(
There was a problem hiding this comment.
It may help to define
constexpr uint16_t stripIndexMask = 0X7FFF;// add a comment here
constexpt uing16_t mergedValueMask = 0X8000;//add a comment here
There was a problem hiding this comment.
ok, given an object xy, w/ x=0 or 1 and y whatever
& 0x7FFF --> return y, setting x to 0
& 0x8000 --> return x, setting y to 0
|= 0x8000 --> set x to 1, leaving y unchanged
&= 0x7FFF --> set x to 0, leaving y unchanged
right ?
There was a problem hiding this comment.
Hi Mia,
That's right. But in trying to implement Slava's suggested named
constexpr quantities (I'm not familiar with the use of constexpr), in
the header,
static constexpr uint16_t stripIndexMask = 0x7FFF; // The first
strip index is in the low 15 bits of firstStrip_
static constexpr uint16_t mergedValueMask = 0x8000; // The merged
state is given by the high bit of firstStrip_
I'm getting
error: ISO C++ forbids in-class initialization of non-const static
member 'stripIndexMask'
(without "static" it seems to make it static, then give the above errror).
I could put the definitions inside the functions, but that would
duplicate code and defeat the purpose I think.
It does seem to accept
static const uint16_t stripIndexMask = 0x7FFF; // The first strip
index is in the low 15 bits of firstStrip_
static const uint16_t mergedValueMask = 0x8000; // The merged state
is given by the high bit of firstStrip_
That's what I have at this point.
Bill
On 11/12/2014 12:20 PM, mia tosi wrote:
In DataFormats/SiStripCluster/interface/SiStripCluster.h:
@@ -33,7 +33,7 @@ class SiStripCluster {
/** The number of the first strip in the cluster
*/
- uint16_t firstStrip() const {return firstStrip_;}
- uint16_t firstStrip() const {return firstStrip_ & 0x7FFF;}
ok, given an object xy, w/ x=0 or 1 and y whatever
& 0x7FFF --> return y, setting x to 0
& 0x8000 --> return x, setting y to 0
|= 0x8000 --> set x to 1, leaving y unchanged
&= 0x7FFF --> set x to 0, leaving y unchangedright ?
—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/6349/files#r20242922.
There was a problem hiding this comment.
Hi Bill,
Sorry, I didn't give a verbatim correct const text.
"static const" is ok.
constexpr is from c++11 and I forgot it's in the DataFormats where reflex needs non-c++11 code
|
I guess the solution to this will be to use a conf.existsAs. On 11/12/2014 05:42 AM, cmsbuild wrote:
|
|
I think I've fixed the issues other than removing the illegal file reader. In particular, added protection for missing parameter found in previous test. Perhaps the tests could be rerun to see if we now pass? |
|
Hi Bill, So, I was curious to see the effects of the cluster refinement enabled on events with tracks nearby but with the module occupancy low. I used the replace commands you pointed me to by email I ran it on Zprime (mass 1.5 TeV) decaying to tau tau (2K events in workflow 1345).
So, this is probably not a big deal, but there's still some phase space where there'd still be some loss. |
|
Hi Slava, Interesting; thanks for adding this information. If I go to the relMon Bill On 11/15/14 5:21 PM, Slava Krutelyov wrote:
|
|
+1 |
|
This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes (tests are also fine). This pull request will be automatically merged. |
Flag merged clusters for relaxed CPE errors
|
Hi Bil, Lowering the threshold has implications with respect to pileup. Did you have a chance to get the numbers for the average occupancy for On 11/15/14, 10:02 PM, wmtford wrote:
Vyacheslav (Slava) Krutelyov |
|
[I'll keep reusing this thread] |
|
Slava, you can find figures on the occupancy here: http://venturia.home.cern.ch/venturia/occupancy/occupancy.htm or in the tracking paper. Il giorno 16/nov/2014, alle ore 14:47, Slava Krutelyov <notifications@github.commailto:notifications@github.com> ha scritto: [I'll keep reusing this thread] — |
|
Hi Andrea, Thank you. My back-of-the-envelope estimate was giving 5-10% for PU40. |
|
Hi Slava, I have now managed to run on the TTbar, PU 35 events sample from The average occupancy increases from 1.2% to 3.6%. The average fraction We should keep looking for a smarter strategy. Bill
|
|
Hi Bill, please post a link to the slides |
|
Hi Slava, I edited the comment in github to include the plot. (Sorry I missed how Bill On 11/18/14 3:46 PM, Slava Krutelyov wrote:
|
|
Hi Bill, Thanks, I can see the plot now (it still takes a click to get there, but it works). I guess the occupancy still works as a merged proxy for parts of the detector where occupancies are lower. Some combination of width, amplitude and local occupancy could probably be made into a better selector. But this, I guess, will require some more effort and time. |
Add a function "refineCluster" in SiStripClusterizer to mark SiStrip clusters as merged. Use this information StripCPEfromTrackAngle to fall back to the larger "legacy" CPE errors for these clusters. The flag lives on the high bit of the firstStrip_ private data member of the SiStripCluster. Configurable thresholds in the sensor occupancy and strip width are used to discriminate between un- and merged candidates. The cluster refiner is turned off as configured for this PR. (Note that it's also overridden by useLegacyError = cms.bool(True) in StripCPEfromTrackingAngle_cfi.)
I'm closing PR #6348, which was based on 7_2_0_pre6, in favor of this one, based on 7_3_0_pre2, though I haven't tested execution in this release.