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

Bad FastSimEvent dependency in reco-ntuple. #60

Open
rovere opened this issue Aug 29, 2018 · 9 comments
Open

Bad FastSimEvent dependency in reco-ntuple. #60

rovere opened this issue Aug 29, 2018 · 9 comments

Comments

@rovere
Copy link
Contributor

rovere commented Aug 29, 2018

I'm not sure why we had to reuse FastSimulation tools inside the reco-ntuple.
Inspired by the problem Jeremy reported last Monday, I discovered that the FastSimEvent does implicitly make assumptions (hardcoded, of course) on the geometry that are indeed false and meaningless for the HGCAL case.
I'm, in particular, referring to these values that are required from here. This confuses the ntuplizer that will simply skip valid simTracks for no good reasons.
And no, of course they are not configurable.

@clelange
Copy link
Contributor

The use of these tools was introduced by @beaudett since he already knew how to use these tools. Given the issues you point out, I agree that this should be fixed.

@rovere
Copy link
Contributor Author

rovere commented Aug 29, 2018

We could either customize the tool in the official release or find some alternative which, admittedly, will require time.
I'm not familiar with the kind of tools that FastSimEvent provided out of the box that we are using in the ntuplizer. Maybe some hits/guidance from @beaudett (et all, of course), would help and speed up the process.

@beaudett
Copy link
Contributor

beaudett commented Aug 29, 2018 via email

@rovere
Copy link
Contributor Author

rovere commented Aug 30, 2018

Ciao @beaudett ,
this kind of logic ("clever" event analysis) is what I have re-implemented with graphs in the CaloParticles. I'm not sure if we could rely completely on that and, also, if those (the CaloParticles) are covering all the use cases we envisage to study with the ntuplizer.
For the time being, I agree with your suggestions and improve the code in the release.
Thanks for the insight.

@beaudett
Copy link
Contributor

beaudett commented Sep 5, 2018

FYI:
cms-sw/cmssw#24473

@clelange
Copy link
Contributor

clelange commented Oct 8, 2018

Hello @beaudett and @rovere - now that cms-sw/cmssw#24473 is merged (and is in CMSSW_10_3_0_pre5), would you be willing to change the ntupliser configuration/code such that it uses values that make sense?

@beaudett
Copy link
Contributor

beaudett commented Oct 8, 2018

Hello @clelange ,
ok, I'll do it. Since I understood you are working on an updated version, is it fine if I provide you with the two python lines that will go in the config file so that you directly include it in your version ?
F.

@clelange
Copy link
Contributor

clelange commented Oct 8, 2018

Sure, that would be fine.

@beaudett
Copy link
Contributor

beaudett commented Oct 8, 2018

So, I propose to put
process.ana.TestParticleFilter.rMax = cms.double(160.)
process.ana.TestParticleFilter.zMax = cms.double(319.)

The first one is consistent with what is done in the ntuplizer code. The second one should correspond to a position located after the thermal shield. The position of the first silicon layer is 321cm. I don't think we need to be super-precise here.
Thanks.

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

No branches or pull requests

3 participants