New DataFormat for the new Jet Reconstruction method in Heavy Ions #189

yetkinyilmaz · 2013-07-27T14:56:04Z

Part of the implementation for the new jet reconstruction (UE subtraction) software, presented in:
https://indico.cern.ch/conferenceDisplay.py?confId=254369

This is just the new data format (candidate-to-background map), independent of other packages. The actual producers are to be requested in a later pull request.

slava77 · 2013-07-29T12:34:07Z

DataFormats/HeavyIonEvent/interface/Centrality.h

@@ -1,5 +1,5 @@
 //
-// $Id: Centrality.h,v 1.11 2010/08/14 17:06:50 nart Exp $
+// $Id: Centrality.h,v 1.12 2010/08/23 16:42:01 nart Exp $


these should be removed, now after migration to git

nclopezo · 2013-07-29T12:52:01Z

Hi,
I took CMSSW_7_0_X_2013-07-28-0200, pulled these changes, and ran the RelVals and unit tests, all tests passed.

slava77 · 2013-07-29T13:00:34Z

DataFormats/HeavyIonEvent/interface/VoronoiBackground.h

+ // typedef edm::AssociationMap<edm::OneToValue<reco::PFCandidateCollection, reco::VoronoiBackground> > VoronoiBackgroundMap;
+ // typedef edm::AssociationMap<edm::OneToValue<reco::CandidateCollection, reco::VoronoiBackground> > VoronoiBackgroundMap;
+
+ typedef edm::AssociationMap<edm::OneToValue<reco::CandidateView, reco::VoronoiBackground> > VoronoiBackgroundMap;


AssociationMap should be avoided for performance reasons. If every PFCandidate in the reference collection has a VoronoiBackground computed, then use ValueMap

Hi Slava;
I prefered AssociationMap because there was good enough documentation available for it.

https://twiki.cern.ch/twiki/bin/view/CMSPublic/WorkBookAssociationMap

Indeed, ValueMap may be better, but is there such documentation for it as well? Would it be compatible with the cases where the mapped Candidate collection is filtered after creating the map?
Thanks.

On Jul 29, 2013, at 3:00 PM, slava77 wrote:

In DataFormats/HeavyIonEvent/interface/VoronoiBackground.h:

double pt_corrected;

double mt_preeq;

double mt_posteq;

double mt_corrected;

double voronoi_area;

//RefVector of neighbors...

+};
+
+

// typedef edm::AssociationMap<edm::OneToValue<reco::PFCandidateCollection, reco::VoronoiBackground> > VoronoiBackgroundMap;

// typedef edm::AssociationMap<edm::OneToValue<reco::CandidateCollection, reco::VoronoiBackground> > VoronoiBackgroundMap;
+

typedef edm::AssociationMap<edm::OneToValue<reco::CandidateView, reco::VoronoiBackground> > VoronoiBackgroundMap;
AssociationMap should be avoided for performance reasons. If every PFCandidate in the reference collection has a VoronoiBackground computed, then use ValueMap

—
Reply to this email directly or view it on GitHub.

On 7/29/13 3:11 PM, yetkinyilmaz wrote:

In DataFormats/HeavyIonEvent/interface/VoronoiBackground.h:

double pt_corrected;

double mt_preeq;

double mt_posteq;

double mt_corrected;

double voronoi_area;

//RefVector of neighbors...

+};
+
+

// typedef edm::AssociationMap<edm::OneToValue<reco::PFCandidateCollection, reco::VoronoiBackground> > VoronoiBackgroundMap;

// typedef edm::AssociationMap<edm::OneToValue<reco::CandidateCollection, reco::VoronoiBackground> > VoronoiBackgroundMap;
+

typedef edm::AssociationMap<edm::OneToValue<reco::CandidateView, reco::VoronoiBackground> > VoronoiBackgroundMap;

Hi Slava; I prefered AssociationMap because there was good enough
documentation available for it.
https://twiki.cern.ch/twiki/bin/view/CMSPublic/WorkBookAssociationMap
Indeed, ValueMap may be better, but is there such documentation for it
as well?

I'd just pick an example code in lxr, and follow from there.
This should be a simple enough example
https://cmssdt.cern.ch/SDT/lxr/source/RecoEgamma/EgammaIsolationAlgos/plugins/EgammaTowerIsolationProducer.cc#038

Would it be compatible with the cases where the mapped
Candidate collection is filtered after creating the map?

Please clarify how you plan to filter.
Will this be a vector of pointers to an old collection,
or will you store copies of original candidates in a new candidate
collection?
In the latter case, the history is lost and the map will have to be redone
to still be meaningful.

Cheers

--slava

Thanks.
… <#>
On Jul 29, 2013, at 3:00 PM, slava77 wrote: In
DataFormats/HeavyIonEvent/interface/VoronoiBackground.h: > + double
pt_corrected; > + > + double mt_preeq; > + double mt_posteq; > + double
mt_corrected; > + > + double voronoi_area; > + //RefVector of
neighbors... > + > +}; > + > + > + // typedef
edm::AssociationMap<edm::OneToValue<reco::PFCandidateCollection,
reco::VoronoiBackground> > VoronoiBackgroundMap; > + // typedef
edm::AssociationMap<edm::OneToValue<reco::CandidateCollection,
reco::VoronoiBackground> > VoronoiBackgroundMap; > + > + typedef
edm::AssociationMap<edm::OneToValue<reco::CandidateView,
reco::VoronoiBackground> > VoronoiBackgroundMap; AssociationMap should
be avoided for performance reasons. If every PFCandidate in the
reference collection has a VoronoiBackground computed, then use ValueMap
— Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub
https://github.com/cms-sw/cmssw/pull/189/files#r5448919.

Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)

I think this is ok, yeah, the filtering should be done by using Refs, which is fine in this case I think. To my understanding, the ValueMap works with the product key of the Refs, and it should be similar to what we've already implemented - even simpler! I had had a lot of hard time getting the dictionaries of the AssociationMap right...

slava77 · 2013-07-29T13:12:50Z

Looking at the content of this DF package, I think a lot of it needs a rewrite: the CentralityProvider is essentially a producer. It can't stay in the DataFormats. Appropriate algorithms should be implemented outside of the DF.

Since we need to backport to an old release (53X?) we can start with minimal changes and then have the modifications mentioned above implemented in 70X

yetkinyilmaz · 2013-07-29T13:26:51Z

CentralityProvider is not a producer (it is not an ED module at all), it is more of an analysis tool. It cannot go into a producer plugin package (e.g. RecoHI/HiCentralityAlgos), because it is supposed to be included by other analysis code. If we want to move it, we will need a new analysis package, but this will be quite a major change. As you say, for the time being we can let it be as is, and the centrality experts can think about what is needed in re-organization of this, since I believe by now they may have motivations for new updates as well. I will contact them about this.

On Jul 29, 2013, at 3:12 PM, slava77 wrote:

Looking at the content of this DF package, I think a lot of it needs a rewrite: the CentralityProvider is essentially a producer. It can't stay in the DataFormats. Appropriate algorithms should be implemented outside of the DF.

Since we need to backport to an old release (53X?) we can start with minimal changes and then have the modifications mentioned above implemented in 70X

—
Reply to this email directly or view it on GitHub.

yetkinyilmaz · 2013-08-08T10:56:03Z

Hi. I indeed changed to ValueMap.

ktf · 2013-08-09T09:03:19Z

@slava77 can you look if you are happy about these now?

slava77 · 2013-08-09T11:17:17Z

@yetkinyilmaz Please review the two recent comments and apply fixes.
If not too much trouble, please also get rid of the cvs Id lines
Thanks.

cmsbuild · 2013-08-09T11:18:44Z

The following categories have been rejected by @slava77: Reconstruction

@cms-git-reconstruction

yetkinyilmaz · 2013-08-09T11:34:28Z

Yes, I am doing that.

However, in the meanwhile the rest of the reconstruction code got almost ready. Do you think it's a better idea to trash this pull request completely and start with another branch that has all related packages? namely:

DataFormats/HeavyIonEvent
RecoHI/HiJetAlgos
RecoJets/JetProducers

I had told you initially that JetProducers was to be untouched, but I later realized, it has been missing a minor (3 lines removed) but important update:

yetkinyilmaz@6e74af5

i.e. the requirement of jet type is not needed.

On Aug 9, 2013, at 1:17 PM, slava77 wrote:

@yetkinyilmaz Please review the two recent comments and apply fixes.
If not too much trouble, please also get rid of the cvs Id lines
Thanks.

—
Reply to this email directly or view it on GitHub.

slava77 · 2013-08-09T12:09:35Z

HI Sal and Roman,

Could you please comment on possibility to change
RecoJets/JetProducers/plugins/VirtualJetProducer.cc
as in
yetkinyilmaz@6e74af5

Thank you.

Cheers

    --slava

P.S. Feel free to reply to the github comment

On 8/9/13 1:34 PM, yetkinyilmaz wrote:

Yes, I am doing that.

However, in the meanwhile the rest of the reconstruction code got almost
ready. Do you think it's a better idea to trash this pull request
completely and start with another branch that has all related packages?
namely:

DataFormats/HeavyIonEvent
RecoHI/HiJetAlgos
RecoJets/JetProducers

I had told you initially that JetProducers was to be untouched, but I
later realized, it has been missing a minor (3 lines removed) but
important update:

yetkinyilmaz@6e74af5

i.e. the requirement of jet type is not needed.

On Aug 9, 2013, at 1:17 PM, slava77 wrote:

@yetkinyilmaz Please review the two recent comments and apply fixes.
If not too much trouble, please also get rid of the cvs Id lines
Thanks.

—
Reply to this email directly or view it on GitHub.

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

Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)

slava77 · 2013-08-09T12:14:58Z

Hi Yetkin,

Thanks for the follow up.

I think you can as well merge the rest together with the data formats.

Considering the coding violations in the DF and trying to minimize the backport volume,
I suggest to do the following:

once you have your producers ready, make a pull request based on that to your target production release branch (53X, is it?)
after that fix the code violations in the DF package (move all algorithmic stuff outside of DF, see https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideCreatingNewProducts for possible dependencies in a DF subsystem)
this fixed version can then go to the development cycle (70X)
We can discuss this at the release planning today

slava77 · 2013-08-09T12:18:26Z

@slava77 test

rappoccio · 2013-08-09T13:31:54Z

The changes to VirtualJetProducer should be fine for pp running, this bit isn't used at all.

slava77 · 2013-08-09T16:50:41Z

another test

ktf · 2013-08-16T08:25:27Z

@yetkinyilmaz any further development for this?

yetkinyilmaz · 2013-08-16T09:21:50Z

Hi.
We had changed the plans a bit, there will be more updates before this branch is merged with 7_X. This will take some time, I am estimating like ~2 weeks, since the developers are more urgently focused on the 5_3_X implementation. Therefore if you prefer, we can cancel this merge request and come up with a new one when it's ready.

On Aug 16, 2013, at 10:25 AM, Giulio Eulisse wrote:

@yetkinyilmaz any further development for this?

—
Reply to this email directly or view it on GitHub.

slava77 · 2013-08-16T09:26:23Z

Hi Yetkin,

You should be able to close it, id that's desired.

If the following additions will be based on this pull request commits,
I'd rather this sticks around as rejected and modified later,
but it depends on your development area/preference,
if you would rather start from scratch.
If it stays,
Giulio may want to change the milestone to pre4 to match the ~2 weeks ETA.

Cheers

    --slava

On 8/16/13 11:21 AM, yetkinyilmaz wrote:

Hi.
We had changed the plans a bit, there will be more updates before this
branch is merged with 7_X. This will take some time, I am estimating
like ~2 weeks, since the developers are more urgently focused on the
5_3_X implementation. Therefore if you prefer, we can cancel this merge
request and come up with a new one when it's ready.

On Aug 16, 2013, at 10:25 AM, Giulio Eulisse wrote:

@yetkinyilmaz any further development for this?

—
Reply to this email directly or view it on GitHub.

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

Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)

yetkinyilmaz · 2013-08-16T09:50:34Z

Hi Slava;
yes, rejecting and changing the target is also good, and it's ok to work with the existing branch.

On Aug 16, 2013, at 11:26 AM, slava77 wrote:

Hi Yetkin,

You should be able to close it, id that's desired.

If the following additions will be based on this pull request commits,
I'd rather this sticks around as rejected and modified later,
but it depends on your development area/preference,
if you would rather start from scratch.
If it stays,
Giulio may want to change the milestone to pre4 to match the ~2 weeks ETA.

Cheers

--slava

On 8/16/13 11:21 AM, yetkinyilmaz wrote:

Hi.
We had changed the plans a bit, there will be more updates before this
branch is merged with 7_X. This will take some time, I am estimating
like ~2 weeks, since the developers are more urgently focused on the
5_3_X implementation. Therefore if you prefer, we can cancel this merge
request and come up with a new one when it's ready.

On Aug 16, 2013, at 10:25 AM, Giulio Eulisse wrote:

@yetkinyilmaz any further development for this?

—
Reply to this email directly or view it on GitHub.

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

Vyacheslav (Slava) Krutelyov
TAMU: Physics Dept Texas A&M MS4242, College Station, TX 77843-4242
CERN: 42-R-027
AIM/Skype: siava16 googleTalk: slava77@gmail.com
(630) 291-5128 Cell (US) +41 76 275 7116 Cell (CERN)

—
Reply to this email directly or view it on GitHub.

ktf · 2013-09-19T09:45:56Z

Can I just close this?

yetkinyilmaz · 2013-09-19T10:17:21Z

Yes, Giulio, please close this, we will have a completely different branch for the pull request.
Sorry, barely getting used to git, but hopefully soon we will converge.

On Sep 19, 2013, at 11:45 AM, Giulio Eulisse notifications@github.com wrote:

Can I just close this?

—
Reply to this email directly or view it on GitHub.

Add SimCalorimetry/CastorTechTrigProducer.

Migrate single lepton cfg and tree producer to Heppy

Adding tkMetPVchs_phi and metNoPU_phi

Taunu and hmm

…-sw#189) * Changes from Anders * code-formatted

added Voronoi Background objects

109c54d

slava77 reviewed Jul 29, 2013
View reviewed changes

ghost assigned slava77 Aug 5, 2013

Changed to ValueMap

8d5b9b9

ktf closed this Sep 19, 2013

This was referenced Nov 29, 2013

Heavy Ion re-reco software #1564

Merged

fix code violations in DataFormats/HeavyIonEvent Centrality and CentralityProvider slava77/cmssw#15

Closed

nclopezo pushed a commit to nclopezo/cmssw that referenced this pull request Nov 21, 2014

Merge pull request cms-sw#189 from ktf/add-missing-package

0b231b1

Add SimCalorimetry/CastorTechTrigProducer.

parbol pushed a commit to parbol/cmssw that referenced this pull request Mar 5, 2015

Merge pull request cms-sw#189 from safarzad/CMGTools-from-CMSSW_7_2_3

ddc87f1

Migrate single lepton cfg and tree producer to Heppy

arizzi added a commit to arizzi/cmssw that referenced this pull request Sep 30, 2015

Merge pull request cms-sw#189 from silviodonato/patch-5

0010d59

Adding tkMetPVchs_phi and metNoPU_phi

mariadalfonso pushed a commit to mariadalfonso/cmssw that referenced this pull request Jan 26, 2018

Merge pull request cms-sw#189 from amarini/taunu_and_hmm

62d7bc3

Taunu and hmm

mandrenguyen added a commit to mandrenguyen/cmssw that referenced this pull request Mar 18, 2019

More accurate ecal scale values (cms-sw#189)

0eee47b

emily-tsai11 pushed a commit to emily-tsai11/cmssw that referenced this pull request Nov 15, 2022

Produce combined module MemPrint & LUT files for reduced project (cms…

5e167c3

…-sw#189) * Changes from Anders * code-formatted

rgoldouz pushed a commit to rgoldouz/cmssw that referenced this pull request Dec 15, 2022

Produce combined module MemPrint & LUT files for reduced project (cms…

4dc5a44

…-sw#189) * Changes from Anders * code-formatted

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New DataFormat for the new Jet Reconstruction method in Heavy Ions #189

New DataFormat for the new Jet Reconstruction method in Heavy Ions #189

yetkinyilmaz commented Jul 27, 2013

slava77 Jul 29, 2013

nclopezo commented Jul 29, 2013

slava77 Jul 29, 2013

yetkinyilmaz Jul 29, 2013

slava77 Jul 29, 2013

yetkinyilmaz Jul 29, 2013

slava77 commented Jul 29, 2013

yetkinyilmaz commented Jul 29, 2013

yetkinyilmaz commented Aug 8, 2013

ktf commented Aug 9, 2013

slava77 commented Aug 9, 2013

cmsbuild commented Aug 9, 2013

yetkinyilmaz commented Aug 9, 2013

slava77 commented Aug 9, 2013

slava77 commented Aug 9, 2013

slava77 commented Aug 9, 2013

rappoccio commented Aug 9, 2013

slava77 commented Aug 9, 2013

ktf commented Aug 16, 2013

yetkinyilmaz commented Aug 16, 2013

slava77 commented Aug 16, 2013

yetkinyilmaz commented Aug 16, 2013

ktf commented Sep 19, 2013

yetkinyilmaz commented Sep 19, 2013

New DataFormat for the new Jet Reconstruction method in Heavy Ions #189

New DataFormat for the new Jet Reconstruction method in Heavy Ions #189

Conversation

yetkinyilmaz commented Jul 27, 2013

slava77 Jul 29, 2013

Choose a reason for hiding this comment

nclopezo commented Jul 29, 2013

slava77 Jul 29, 2013

Choose a reason for hiding this comment

yetkinyilmaz Jul 29, 2013

Choose a reason for hiding this comment

slava77 Jul 29, 2013

Choose a reason for hiding this comment

yetkinyilmaz Jul 29, 2013

Choose a reason for hiding this comment

slava77 commented Jul 29, 2013

yetkinyilmaz commented Jul 29, 2013

yetkinyilmaz commented Aug 8, 2013

ktf commented Aug 9, 2013

slava77 commented Aug 9, 2013

cmsbuild commented Aug 9, 2013

yetkinyilmaz commented Aug 9, 2013

slava77 commented Aug 9, 2013

slava77 commented Aug 9, 2013

slava77 commented Aug 9, 2013

rappoccio commented Aug 9, 2013

slava77 commented Aug 9, 2013

ktf commented Aug 16, 2013

yetkinyilmaz commented Aug 16, 2013

slava77 commented Aug 16, 2013

yetkinyilmaz commented Aug 16, 2013

ktf commented Sep 19, 2013

yetkinyilmaz commented Sep 19, 2013