Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heavy Ion re-reco software #1564

Merged
merged 32 commits into from Feb 26, 2014
Merged

Conversation

yetkinyilmaz
Copy link
Contributor

Reconstruction updates for jets and tracking.

Ignore *.C root macro files in test/ directories, which are for user-level analyses.

The RelVal tests will crash because of not having the proper L1 menu in the available global tag compatible with HLT.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @yetkinyilmaz for CMSSW_5_3_X.

Heavy Ion re-reco software

It involves the following packages:

RecoHI/HiJetAlgos
RecoHI/HiEgammaAlgos
RecoJets/JetProducers
RecoHI/HiCentralityAlgos
RecoHI/Configuration
RecoHI/HiTracking
DataFormats/HeavyIonEvent
RecoHI/HiMuonAlgos
Validation/RecoHI

@nclopezo, @cmsbuild, @thspeer, @slava77 can you please review it and eventually sign? Thanks.
@ferencek, @TaiSakuma 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.
@smuzaffar you are the release manager for this.

@smuzaffar
Copy link
Contributor

+tested

@slava77
Copy link
Contributor

slava77 commented Nov 29, 2013

@slava77 started working on this


protected:

double pt_preeq;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doubles?
Can we use float precision here?
Please change to float if doubles are not justified

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi.
I think that's because the standard reco::Candidate (and math:: LorentzVector) are in double, and this number is to replace the pt of candidates.
I'm not sure if using float would be a problem either. Do you have a strong opinion?
Thanks.

On Nov 29, 2013, at 1:06 PM, slava77 notifications@github.com wrote:

In DataFormats/HeavyIonEvent/interface/VoronoiBackground.h:

+#include
+#include
+
+namespace reco { class VoronoiBackground {
+public:

  •  VoronoiBackground();
    
  •  VoronoiBackground(double pt0, double pt1, double pt2, double mt0, double mt1, double mt2);
    
  •  virtual ~VoronoiBackground();
    
  • double pt() const{
  • return pt_corrected;
    
  • }

+protected:
+

  • double pt_preeq;
    Why doubles?
    Can we use float precision here?
    Please change to float if doubles are not justified


Reply to this email directly or view it on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no strong opinion.
ok

@slava77
Copy link
Contributor

slava77 commented Nov 29, 2013

@yetkinyilmaz
Hi Yetkin

We had a pretty long discussion related to Centrality, see comments in #189
What's the progress on that end?
At some point I understood that one of the reasons it took some time to get from #189 to this PR was to also fix the centrality.

Please comment on your progress or plans with that.

Thanks.

@yetkinyilmaz
Copy link
Contributor Author

I think the centrality team was working on this, although a bit slowly. I will ping them again about this. But if you see message:

#189 (comment)

the conclusion was that we must fix these issues for the actual 7_0_X development, and for 5_3_X try to survive with this part as it is.

On the other hand, if the fixes are ready early enough (by any chance), it would be very straightforward to apply them in this pull request as well, and in that case we should do it.

(A question: Is there a way to ensure to notify the relevant people automatically for the pull request of packages they are responsible of? I'm not sure who are getting these messages)

On Nov 29, 2013, at 1:32 PM, slava77 notifications@github.com wrote:

@yetkinyilmaz
Hi Yetkin

We had a pretty long discussion related to Centrality, see comments in #189
What's the progress on that end?
At some point I understood that one of the reasons it took some time to get from #189 to this PR was to also fix the centrality.

Please comment on your progress or plans with that.

Thanks.


Reply to this email directly or view it on GitHub.

* hiEcalClusters
* hiRecoJets
* muonRecoPbPb
* reMuonRecoPbPb
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remind me why do we need to run these two versions here now?
At least the naming is pretty odd now (re-reco followed immediately after the regular reco).

Do you have some slides summarizing all the changes in the HI RECO?
Some of it so far looks like a temporary patch-work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this pull request has been a good exercise - I really need to learn how make the relevant parties participate in this discussion.
I am rather related to the jet developments, but in the new reconstruction we had also updates on tracking and muons, which I only tested without going into the details of the development. I will try to figure these out.

On Nov 29, 2013, at 1:50 PM, slava77 notifications@github.com wrote:

In RecoHI/Configuration/python/Reconstruction_HI_cff.py:

                           * hiEcalClusters
                           * hiRecoJets
                           * muonRecoPbPb
  •                          \* reMuonRecoPbPb
    
    Please remind me why do we need to run these two versions here now?
    At least the naming is pretty odd now (re-reco followed immediately after the regular reco).

Do you have some slides summarizing all the changes in the HI RECO?
Some of it so far looks like a temporary patch-work.


Reply to this email directly or view it on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yetkin you can mention people in the discussion prefixing with "@" their username e.g. "@yetkinyilmaz" and they'll be automatically notified. You can also ask to watch specific packages and you and your colleagues will receive a notification when a given package is touched. Just create an issue in https://github.com/cms-sw/cmssw/issues/new and specify who should watch which package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so I change the name reMuonRecoPbPb to regionalMuonRecoPbPb, good? @MiheeJo @mandrenguyen ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On 12/16/13, 3:17 PM, yetkinyilmaz wrote:

In RecoHI/Configuration/python/Reconstruction_HI_cff.py:

                           * hiEcalClusters
                           * hiRecoJets
                           * muonRecoPbPb
  •                          \* reMuonRecoPbPb
    

Ok, so I change the name reMuonRecoPbPb to regionalMuonRecoPbPb, good?

Sounds good to me.

    --slava

@MiheeJo https://github.com/miheejo @mandrenguyen
https://github.com/mandrenguyen ?


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


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
Copy link
Contributor

slava77 commented Nov 29, 2013

I have a few general questions to the new code, use of BPMPD and
RecoHI/HiJetAlgos/interface/VoronoiAlgorithm.h, which contains quite a bit of overhead class and method definitions, including
- bpmpd_problem_t : public problem_t (class structure unclear, as if multiple problem/solver definitions are expected)
- snowmass_vector_t -- this is just a redefinition of

On the VoronoiAlgorithm (and related files)

  • Is this code supposed to stay in CMSSW (or is it something that's planned to be added to some external package, like fastjet or similar)?
  • If we keep it in CMSSW, the code should follow CMSSW style conventions http://cmsdoc.cern.ch/cms/cpt/Software/html/General/CodingAndStyleRules.pdf (capitalization, separate classes in separate files, all complex method should be in .cc files)
  • if this code is to be used just locally, please move it to /src and include there, so that these interfaces are not exposed

On the use of BPMPD

  • this version is dated 1996, is there anything more fresh (is there anything not in FORTRAN)? version 2.21?
  • did you consider any other solvers ? (is there anything in CGAL?)
  • what are the performance gains from using this package compared to other ones?
  • considering that this is fortran and that quite likely the code is fairly heavy CPU-wise, it will become a bottleneck in multithreading environment. We should avoid this.

I suggest that you prepare some details on implementation and present in the next RECO meeting.

@yetkinyilmaz
Copy link
Contributor Author

Thanks a lot for the comments Slava,
we will sort these out with @yslai and @mandrenguyen
It would be great if Yue Shi is available at cern for the RECO meeting, otherwise one of us will try to connect remotely.

On Nov 29, 2013, at 6:13 PM, slava77 notifications@github.com wrote:

I have a few general questions to the new code, use of BPMPD and
RecoHI/HiJetAlgos/interface/VoronoiAlgorithm.h, which contains quite a bit of overhead class and method definitions, including

  • bpmpd_problem_t : public problem_t (class structure unclear, as if multiple problem/solver definitions are expected)
  • snowmass_vector_t -- this is just a redefinition of

On the VoronoiAlgorithm (and related files)

Is this code supposed to stay in CMSSW (or is it something that's planned to be added to some external package, like fastjet or similar)?
If we keep it in CMSSW, the code should follow CMSSW style conventions http://cmsdoc.cern.ch/cms/cpt/Software/html/General/CodingAndStyleRules.pdf (capitalization, separate classes in separate files, all complex method should be in .cc files)
if this code is to be used just locally, please move it to /src and include there, so that these interfaces are not exposed
On the use of BPMPD

this version is dated 1996, is there anything more fresh (is there anything not in FORTRAN)? version 2.21?
did you consider any other solvers ? (is there anything in CGAL?)
what are the performance gains from using this package compared to other ones?
considering that this is fortran and that quite likely the code is fairly heavy CPU-wise, it will become a bottleneck in multithreading environment. We should avoid this.
I suggest that you prepare some details on implementation and present in the next RECO meeting.


Reply to this email directly or view it on GitHub.

@@ -4,12 +4,8 @@
<use name="FWCore/ParameterSet"/>
<use name="RecoHI/HiJetAlgos"/>
<use name="DataFormats/JetReco"/>
<Flags CXXFLAGS="-frounding-math"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this required here?
@smuzaffar @ktf
Will this work properly on all architecture that we support?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, just realized that this point is not resolved, can @yslai please comment on the first question and @smuzaffar and @ktf comment on the second?
Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is required due to CGAL. While we do not use rounding-sensitive parts
of CGAL (the Voronoi diagram construction is safe in that regard),
unfortunately CGAL checks during library initialization that
-frounding-math is active. So short of an invasive patching of CGAL this
is the simplest solution.

-frounding-math only affects the optimization, and is portable for all
architectures supported by GCC.

On 29.11.2013 18:35, slava77 wrote:

In RecoHI/HiJetAlgos/plugins/BuildFile.xml:

@@ -4,12 +4,8 @@



+

Is this required here?
@smuzaffar https://github.com/smuzaffar @ktf https://github.com/ktf
Will this work properly on all architecture that we support?


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

@cmsbuild
Copy link
Contributor

Pull request #1564 was updated. @thspeer, @danduggan, @rovere, @cmsbuild, @anton-a, @nclopezo, @deguio, @slava77, @Degano, @ojeda can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Feb 25, 2014

So, I'm told this is the last one ;)
testing 880390e now in CMSSW_5_3_X_2014-02-24-0200

@slava77
Copy link
Contributor

slava77 commented Feb 25, 2014

+1

for #1564 880390e
tested in CMSSW_5_3_X_2014-02-24-0200/sign5314b3

Based mainly on the discussion above (tests done in 5_3_14), and a shorter set of tests in 53X IB.

The last iteration of fixes has the same event content, the timing went up a bit
these are expected for voronoi bgd (more complex parameterization here).

The calibration files are read from the external as expected.

@danduggan
Copy link
Contributor

+1
resigning after the update.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_5_3_X IBs unless changes or unless it breaks tests. @smuzaffar can you please take care of it?

@davidlange6
Copy link
Contributor

+1

smuzaffar added a commit that referenced this pull request Feb 26, 2014
@smuzaffar smuzaffar merged commit 589a612 into cms-sw:CMSSW_5_3_X Feb 26, 2014
@smuzaffar
Copy link
Contributor

@yetkinyilmaz
After merging this request we get duplicate dictionaries

pluginRecoHIHiEvtPlaneAlgos.so CentralityPopConProducer CMS%EDM%Framework%Module pluginHiCentralityPlugins.so CentralityPopConProducer CMS%EDM%Framework%Module pluginHiCentralityPlugins.so CentralityPopConProducer CMS%EDM%Framework%ParameterSet%Description pluginHiCentralityPlugins.so AnalyzerWithCentrality CMS%EDM%Framework%ParameterSet%Description pluginRecoHIHiEvtPlaneAlgos.so CentralityPopConProducer CMS%EDM%Framework%ParameterSet%Description pluginHiCentralityTestPlugins.so AnalyzerWithCentrality CMS%EDM%Framework%ParameterSet%Description pluginHiCentralityTestPlugins.so AnalyzerWithCentrality CMS%EDM%Framework%Module pluginHiCentralityPlugins.so AnalyzerWithCentrality CMS%EDM%Framework%Module

You have DEFINE_FWK_MODULE(AnalyzerWithCentrality); both in test and plugins subdir and
DEFINE_FWK_MODULE(CentralityPopConProducer); in src dirctory.

I would suggest that please remove the test/AnalyzerWithCentrality.cc and test/BuildFile.xml
and move src/CentralityTableHandler.cc in to plugins directory .

@yetkinyilmaz
Copy link
Contributor Author

Now that 1564 is merged, this should be done in a new pull-request right?

On Feb 27, 2014, at 10:51 AM, Malik Shahzad Muzaffar notifications@github.com wrote:

After merging this request we get duplicate dictionaries

pluginRecoHIHiEvtPlaneAlgos.so CentralityPopConProducer CMS%EDM%Framework%Module
pluginHiCentralityPlugins.so CentralityPopConProducer CMS%EDM%Framework%Module
pluginHiCentralityPlugins.so CentralityPopConProducer CMS%EDM%Framework%ParameterSet%Description
pluginHiCentralityPlugins.so AnalyzerWithCentrality CMS%EDM%Framework%ParameterSet%Description
pluginRecoHIHiEvtPlaneAlgos.so CentralityPopConProducer CMS%EDM%Framework%ParameterSet%Description
pluginHiCentralityTestPlugins.so AnalyzerWithCentrality CMS%EDM%Framework%ParameterSet%Description
pluginHiCentralityTestPlugins.so AnalyzerWithCentrality CMS%EDM%Framework%Module
pluginHiCentralityPlugins.so AnalyzerWithCentrality CMS%EDM%Framework%Module

You have DEFINE_FWK_MODULE(AnalyzerWithCentrality); both in test and plugins subdir and
DEFINE_FWK_MODULE(CentralityPopConProducer); in src dirctory.

I would suggest that please remove the test/AnalyzerWithCentrality.cc and test/BuildFile.xml
and move src/CentralityTableHandler.cc in to plugins directory .


Reply to this email directly or view it on GitHub.

@smuzaffar
Copy link
Contributor

yes
On 02/27/2014 11:49 AM, yetkinyilmaz wrote:

Now that 1564 is merged, this should be done in a new pull-request right?

On Feb 27, 2014, at 10:51 AM, Malik Shahzad Muzaffar
notifications@github.com wrote:

After merging this request we get duplicate dictionaries

pluginRecoHIHiEvtPlaneAlgos.so CentralityPopConProducer
CMS%EDM%Framework%Module
pluginHiCentralityPlugins.so CentralityPopConProducer
CMS%EDM%Framework%Module
pluginHiCentralityPlugins.so CentralityPopConProducer
CMS%EDM%Framework%ParameterSet%Description
pluginHiCentralityPlugins.so AnalyzerWithCentrality
CMS%EDM%Framework%ParameterSet%Description
pluginRecoHIHiEvtPlaneAlgos.so CentralityPopConProducer
CMS%EDM%Framework%ParameterSet%Description
pluginHiCentralityTestPlugins.so AnalyzerWithCentrality
CMS%EDM%Framework%ParameterSet%Description
pluginHiCentralityTestPlugins.so AnalyzerWithCentrality
CMS%EDM%Framework%Module
pluginHiCentralityPlugins.so AnalyzerWithCentrality
CMS%EDM%Framework%Module

You have DEFINE_FWK_MODULE(AnalyzerWithCentrality); both in test and
plugins subdir and
DEFINE_FWK_MODULE(CentralityPopConProducer); in src dirctory.

I would suggest that please remove the
test/AnalyzerWithCentrality.cc and test/BuildFile.xml
and move src/CentralityTableHandler.cc in to plugins directory .


Reply to this email directly or view it on GitHub.


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

@davidlange6
Copy link
Contributor

Yes.

On Feb 27, 2014, at 11:49 AM, yetkinyilmaz notifications@github.com
wrote:

Now that 1564 is merged, this should be done in a new pull-request right?

On Feb 27, 2014, at 10:51 AM, Malik Shahzad Muzaffar notifications@github.com wrote:

After merging this request we get duplicate dictionaries

pluginRecoHIHiEvtPlaneAlgos.so CentralityPopConProducer CMS%EDM%Framework%Module
pluginHiCentralityPlugins.so CentralityPopConProducer CMS%EDM%Framework%Module
pluginHiCentralityPlugins.so CentralityPopConProducer CMS%EDM%Framework%ParameterSet%Description
pluginHiCentralityPlugins.so AnalyzerWithCentrality CMS%EDM%Framework%ParameterSet%Description
pluginRecoHIHiEvtPlaneAlgos.so CentralityPopConProducer CMS%EDM%Framework%ParameterSet%Description
pluginHiCentralityTestPlugins.so AnalyzerWithCentrality CMS%EDM%Framework%ParameterSet%Description
pluginHiCentralityTestPlugins.so AnalyzerWithCentrality CMS%EDM%Framework%Module
pluginHiCentralityPlugins.so AnalyzerWithCentrality CMS%EDM%Framework%Module

You have DEFINE_FWK_MODULE(AnalyzerWithCentrality); both in test and plugins subdir and
DEFINE_FWK_MODULE(CentralityPopConProducer); in src dirctory.

I would suggest that please remove the test/AnalyzerWithCentrality.cc and test/BuildFile.xml
and move src/CentralityTableHandler.cc in to plugins directory .


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet