Fixed new fast pv3 #2042

perrotta · 2014-01-16T11:10:54Z

It is the same as the closed PR #1918 (by @silviodonato and @arizzi):

definition of two new producer modules, one for fast primary vertex in RecoPixelVertexing/PixelVertexFinding and one jet producer in HLTrigger/JetMET, in view of their usage for forthcoming HLT developments

It also takes into account all the fixes proposed by Slava, including the displacement of the jet producer PixelJetPuId to HLTrigger/JetMET.

It is safe to include it in the 7_0_X release as none of the present modules or classes gets ouched, and none of the possible present workflows will be affected. For the same reason, it cannot be tested with the present workflows or addon tests (this is just FYI).

cmsbuild · 2014-01-16T11:13:05Z

A new Pull Request was created by @perrotta for CMSSW_7_0_X.

Fixed new fast pv3

It involves the following packages:

HLTrigger/JetMET
RecoPixelVertexing/PixelVertexFinding

@perrotta, @cmsbuild, @thspeer, @fwyzard, @Martin-Grunewald, @anton-a, @nclopezo, @slava77, @Degano can you please review it and eventually sign? Thanks.
@cerati, @GiacomoSguazzoni, @rovere this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@ktf you are the release manager for this.

perrotta · 2014-01-16T11:14:34Z

+1

anton-a · 2014-01-16T21:49:25Z

@Degano, @nclopezo can you please run jenkins for this PR

anton-a · 2014-01-16T22:27:23Z

Looks like the proposed changes were implemented.
Still one question. It looks like you are splitting a CaloJet collection into two. What is the reason for saving copies of the jets instead of Refs?

silviodonato · 2014-01-17T10:50:35Z

Which one collection do you prefer?
We don't have any particular preference (it's ok for us to change it to
a Ref vector or similar).

On 01/16/2014 11:27 PM, anton-a wrote:

Looks like the proposed changes were implemented.
Still one question. It looks like you are splitting a CaloJet
collection into two. What is the reason for saving copies of the jets
instead of Refs?

—
Reply to this email directly or view it on GitHub
#2042 (comment).

arizzi · 2014-01-17T11:08:26Z

i.e. better TRefVector, or vectoredm::Ptr or what else?

On Fri, Jan 17, 2014 at 11:50 AM, silviodonato notifications@github.comwrote:

Which one collection do you prefer?
We don't have any particular preference (it's ok for us to change it to
a Ref vector or similar).

On 01/16/2014 11:27 PM, anton-a wrote:

Looks like the proposed changes were implemented.
Still one question. It looks like you are splitting a CaloJet
collection into two. What is the reason for saving copies of the jets
instead of Refs?

—
Reply to this email directly or view it on GitHub
#2042 (comment).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2042#issuecomment-32596139
.

cmsbuild · 2014-01-17T11:53:26Z

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-2042/2122/summary.html

anton-a · 2014-01-17T19:15:09Z

RefVector should be fine for this application

arizzi · 2014-01-20T14:17:58Z

Hi,
well actually as we use edm::View in input is probably easier to fill a
PtrVector, using edm::View::ptrAt(..)

cheers,
andrea

On Fri, Jan 17, 2014 at 8:15 PM, anton-a notifications@github.com wrote:

RefVector should be fine for this application

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2042#issuecomment-32637403
.

silviodonato · 2014-01-20T15:09:42Z

You can find the new file in:
https://github.com/silviodonato/cmssw/blob/fixedFastPV/HLTrigger/JetMET/src/PixelJetPuId.cc

Here we use Ref of CaloJetCollection as output

On 01/20/2014 03:18 PM, arizzi wrote:

Hi,
well actually as we use edm::View in input is probably easier to fill a
PtrVector, using edm::View::ptrAt(..)

cheers,
andrea

On Fri, Jan 17, 2014 at 8:15 PM, anton-a notifications@github.com
wrote:

RefVector should be fine for this application

—
Reply to this email directly or view it on
GitHubhttps://github.com//pull/2042#issuecomment-32637403
.

—
Reply to this email directly or view it on GitHub
#2042 (comment).

arizzi · 2014-01-20T15:12:38Z

Dear all,
(in particular @slava77,@anton-a)

concerning a better handling of the output (copy by value vs RefVector, or
Ptr or whatever ) of this producer, I'm afraid we cannot do anything
without a general clean-up / policy in the trigger chain.

In particular it seems that most modules used downstream in this path,
would need "by value" see for example:
https://cmssdt.cern.ch/SDT/lxr/source/CommonTools/RecoAlgos/plugins/LargestEtCaloJetSelector.cchttps://cmssdt.cern.ch/SDT/lxr/source/CommonTools/RecoAlgos/plugins/LargestEtCaloJetSelector.cc#019

if we switch to producing RefVectors we may break the usages we have in
mind (i.e. feed this to downstream existing HLT modules).

I suggest we go ahead with what andrea perrotta has in his github branch
(i.e. output by value) and then we try to figure if any output collection
can be used that is compatible with downstream filters/producers.

Ciao
Andrea

On Mon, Jan 20, 2014 at 3:17 PM, Andrea Rizzi andrea.rizzi@cern.ch wrote:

Hi,
well actually as we use edm::View in input is probably easier to fill a
PtrVector, using edm::View::ptrAt(..)

cheers,
andrea

On Fri, Jan 17, 2014 at 8:15 PM, anton-a notifications@github.com wrote:

RefVector should be fine for this application

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2042#issuecomment-32637403
.

fwyzard · 2014-01-20T15:17:34Z

On 20 January 2014 16:12, arizzi notifications@github.com wrote:

I suggest we go ahead with what andrea perrotta has in his github branch
(i.e. output by value) and then we try to figure if any output collection
can be used that is compatible with downstream filters/producers.

And/or we can fix the downstream modules to use Refs/Ptrs, if that helps
either timing or memory consumption.

.A

arizzi · 2014-01-20T15:31:20Z

Hi Andrea,
I agree but as any clean up, it should happen as a campaign rather than
with sparse strict requirements (i.e. being strict on the requirements for
this producer while the current status of the code is not enforcing the
same policy).

In particular it seems to me that standard ObjectSelectors are currently
copying by value and reading without "edm::view" so polymorphic access to
vector and vector<Ptr/Ref> (or Ptr/RefVector) is not doable.

It is up to HLT and Reco people to decide if it is worth imlpementing a
(general) cleanup, but this is not a matter of this producer.

Ciao
Andrea

On Mon, Jan 20, 2014 at 4:17 PM, Andrea Bocci notifications@github.comwrote:

On 20 January 2014 16:12, arizzi notifications@github.com wrote:

I suggest we go ahead with what andrea perrotta has in his github branch
(i.e. output by value) and then we try to figure if any output collection
can be used that is compatible with downstream filters/producers.

And/or we can fix the downstream modules to use Refs/Ptrs, if that helps
either timing or memory consumption.

.A

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2042#issuecomment-32767516
.

anton-a · 2014-01-20T17:30:42Z

The reco conveners would prefer to have a joint review with HLT of the format of input/output collections used in HLT to establish a policy that ensures consistency and good performance. Hope that we can set some reasonable timeline. In light of this, we would not object to the inclusion of this particular PR "as is" for testing.

fwyzard · 2014-01-20T17:36:57Z

Hi Anton,

On 20 January 2014 18:30, anton-a notifications@github.com wrote:

The reco conveners would prefer to have a joint review with HLT of the
format of input/output collections used in HLT to establish a policy that
ensures consistency and good performance. Hope that we can set some
reasonable timeline. In light of this, we would not object to the inclusion
of this particular PR "as is" for testing.

we would be very happy to have a full review of the RECO and HLT code in
view of performance, readability and robustness (the configuration of
particle flow modules comes to mind, but maybe things have improved since I
last checked it).

Would you prefer to have an initial meeting with just the HLT STORM
conveners, or include all the POGs contacts ?

Ciao,
.Andrea

Martin-Grunewald · 2014-01-20T17:38:40Z

Please sign this pull request. We can not wait for reviewing (and possibly changing) RECO+HLT concepts.

anton-a · 2014-01-20T17:39:21Z

+1
with the understanding that the outputs will be revised as part of a reco/hlt review

anton-a · 2014-01-20T17:41:20Z

HI Andrea, all,
We can work the details of the meetings offline.
-Anton

cmsbuild · 2014-01-20T17:43:06Z

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_0_X IBs unless changes (tests are also fine). @ktf can you please take care of it?

davidlange6 · 2014-01-22T14:07:23Z

+1

HLT -- Fixed new fast pv3

perrotta added 8 commits January 15, 2014 19:25

From PR cms-sw#1918, plus first fixes by S. Donato

fcfaa48

further fixes by S. Donato

494d4f9

adjust Buildfile

32ad9c3

clean header's includes

556faa9

derive from EDProducer (since it is a producer)

f3abb7f

fix deltaR

b5db985

move PixelJetPuId.cc to HLTrigger/JetMET

93beb03

remove PixelJetPuId from RecoPixelVertexing/PixelVertexFinding

cdb14c8

ktf added a commit that referenced this pull request Jan 23, 2014

Merge pull request #2042 from perrotta/fixedNewFastPv3

b66f0ba

HLT -- Fixed new fast pv3

ktf merged commit b66f0ba into cms-sw:CMSSW_7_0_X Jan 23, 2014

perrotta deleted the fixedNewFastPv3 branch February 1, 2014 08:01

nclopezo modified the milestones: CMSSW_7_0_0, CMSSW_7_0_0_pre13 Feb 7, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed new fast pv3 #2042

Fixed new fast pv3 #2042

perrotta commented Jan 16, 2014

cmsbuild commented Jan 16, 2014

perrotta commented Jan 16, 2014

anton-a commented Jan 16, 2014

anton-a commented Jan 16, 2014

silviodonato commented Jan 17, 2014

arizzi commented Jan 17, 2014

cmsbuild commented Jan 17, 2014

anton-a commented Jan 17, 2014

arizzi commented Jan 20, 2014

silviodonato commented Jan 20, 2014

arizzi commented Jan 20, 2014

fwyzard commented Jan 20, 2014

arizzi commented Jan 20, 2014

anton-a commented Jan 20, 2014

fwyzard commented Jan 20, 2014

Martin-Grunewald commented Jan 20, 2014

anton-a commented Jan 20, 2014

anton-a commented Jan 20, 2014

cmsbuild commented Jan 20, 2014

davidlange6 commented Jan 22, 2014

Fixed new fast pv3 #2042

Fixed new fast pv3 #2042

Conversation

perrotta commented Jan 16, 2014

cmsbuild commented Jan 16, 2014

perrotta commented Jan 16, 2014

anton-a commented Jan 16, 2014

anton-a commented Jan 16, 2014

silviodonato commented Jan 17, 2014

arizzi commented Jan 17, 2014

cmsbuild commented Jan 17, 2014

anton-a commented Jan 17, 2014

arizzi commented Jan 20, 2014

silviodonato commented Jan 20, 2014

arizzi commented Jan 20, 2014

fwyzard commented Jan 20, 2014

arizzi commented Jan 20, 2014

anton-a commented Jan 20, 2014

fwyzard commented Jan 20, 2014

Martin-Grunewald commented Jan 20, 2014

anton-a commented Jan 20, 2014

anton-a commented Jan 20, 2014

cmsbuild commented Jan 20, 2014

davidlange6 commented Jan 22, 2014