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

RECO Step working for HGCal + Basic CMS PFlow Hooks #4121

Merged
merged 10 commits into from Jun 10, 2014

Conversation

lgray
Copy link
Contributor

@lgray lgray commented Jun 5, 2014

Built ontop of : CMSSW_6_2_X_SLHC_2014-06-04-0200

Includes #4029 from @vandreev11

Tested in 12200 and 12219.

Right now there is a bug in the HGC geometry, all silicon cells are given coordinates (0,0,0) this causes the energy-in-MIPs when selecting hits to always be zero, resulting in an empty list of PFClusters since none are selected.

@pfs You might be interested in this!

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2014

A new Pull Request was created by @lgray (Lindsey Gray) for CMSSW_6_2_X_SLHC.

RECO Step working for HGCal + Basic CMS PFlow Hooks

It involves the following packages:

DataFormats/CaloRecHit
DataFormats/ParticleFlowReco
RecoEgamma/EgammaIsolationAlgos
RecoLocalCalo/CaloTowersCreator
RecoLocalCalo/Configuration
RecoLocalCalo/HGCalRecAlgos
RecoLocalCalo/HGCalRecProducers
RecoParticleFlow/Configuration
RecoParticleFlow/PFClusterProducer
SLHCUpgradeSimulations/Configuration

The following packages do not have a category, yet:

RecoLocalCalo/HGCalRecAlgos
RecoLocalCalo/HGCalRecProducers

@civanch, @nclopezo, @mdhildreth, @cmsbuild, @StoyanStoynev, @slava77, @Degano, @ktf can you please review it and eventually sign? Thanks.
@bachtis, @argiro 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.
@fratnikov, @mark-grimes you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@pfs
Copy link
Contributor

pfs commented Jun 5, 2014

@lgray Great work.
Which bug in the geometry are you referring to?

@lgray
Copy link
Contributor Author

lgray commented Jun 5, 2014

@pfs The geometry always returns a position for the HGCal Cell of (0,0,0) @bsunanda is looking into it.

This causes no PF RecHits to be selected since coth(0) = infinity, which then multiplies the MIP value as a function of eta... I could change this to use a non-eta-scaling value for the MIP but it merely hides the issue that the geometry is giving an incorrect value for the rechit position.

@lgray
Copy link
Contributor Author

lgray commented Jun 5, 2014

@pfs I should point out, this is for the RECO geometry only. The simulation geometry appears to be ok.

@bsunanda
Copy link
Contributor

bsunanda commented Jun 6, 2014

Hi Lindsay

Please try the new pull request: bsunanda:Phase2-hgx10. I have not yet
looked into the issue of invalid DetId's yet. But whe the pointer to
geometry is OK it returns some non-zero value for getPosition and if I do
getPosition and getClosestCell, I get back the same DetId. I believe this
is a sensible check. Sorry for the first iteration.

Sunanda

On Thu, 5 Jun 2014, Lindsey Gray wrote:

@pfs I should point out, this is for the RECO geometry only. The simulation
geometry appears to be ok.


Reply to this email directly or view it onGitHub.[5033146__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcxNzYyMT
I2MCwiZGF0YSI6eyJpZCI6MzM5MzI4NDl9fQ==--cea5eb78157e259682b2602a3aaa69dd3e4
9ee13.gif]

@lgray
Copy link
Contributor Author

lgray commented Jun 6, 2014

@bsunanda Great, thanks! I will try it out when I get to CERN.

@bsunanda
Copy link
Contributor

bsunanda commented Jun 6, 2014

Hi Lindsey

There is something wrong with the SIM-DIGI-RECO output. The layer numbers
of all the sample ID's are illegal. May be I shall examine all the steps
in Digi/Reco to see where the DetId gets screwed up. Regards

Sunanda

On Fri, 6 Jun 2014, Lindsey Gray wrote:

@bsunanda Great, thanks! I will try it out when I get to CERN.


Reply to this email directly or view it onGitHub.[5033146__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcxNzY1Nj
A3OCwiZGF0YSI6eyJpZCI6MzM5MzI4NDl9fQ==--5831a5aa9a6e8e13a21908c6443102bbb25
1068e.gif]

@lgray
Copy link
Contributor Author

lgray commented Jun 6, 2014

@bsunanda Ok, thanks for letting me know!

@vandreev11
Copy link
Contributor

@bsunanda, @pfs, @lgray Hi all, I tried to check if there is screw-up in detId when going from
Digis to RecHits. The propagation of detId from digihit to rechit is correct. I found though a few problems with detId of digihits. HGCEE has 32 layers, it should be 31 only. Then HGCHEF digihits come with 32 layers,
it should be 12. Another observation, for 10 muon events workflow 12200 I did not see any subsector2 in HGCHEB, this might be due to low statistics, to watch. The log file of my test can be seen in the
/afs/cern.ch/user/v/vandreev/public/HGC/detId directory.
Best,
Valeri

@vandreev11
Copy link
Contributor

@bsunanda, @pfs Sorry, it is my confusion or misunderstanding on subsector2. I would expect in HEback subsector1/2 for even/odd layers or viseversa. It is always subsector1 in those events.

@lgray
Copy link
Contributor Author

lgray commented Jun 8, 2014

@fratnikov @mark-grimes can we please get this in the release asap (after usual tests, of course). Having to recompile all of CMSSW when the IB becomes outdated is a significant time sink and slows further development.

@lgray
Copy link
Contributor Author

lgray commented Jun 9, 2014

@ktf Is there something wrong with this PR? I saw some issues of others' PRs not being registered by github, etc. It's rather important that this gets into an SLHC IB.

@ktf
Copy link
Contributor

ktf commented Jun 9, 2014

How do you mean? Apart from the new packages (do you really need 2, BTW?), everything else seems to be fine.

@lgray
Copy link
Contributor Author

lgray commented Jun 9, 2014

@ktf New packages are mirroring the structure of the packages in this directory, can change if you think it's necessary? Was referring to the issue with #4129. If everything is otherwise OK I'll stop making noise.

@mark-grimes
Copy link

merge

notes on #4176

cmsbuild added a commit that referenced this pull request Jun 10, 2014
RECO Step working for HGCal + Basic CMS PFlow Hooks
@cmsbuild cmsbuild merged commit e39a217 into cms-sw:CMSSW_6_2_X_SLHC Jun 10, 2014
@mark-grimes mark-grimes mentioned this pull request Jun 10, 2014
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

7 participants