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

change primaryVertexCut for displaced Vertex reconstruction from 2.5 … #9454

Merged
merged 1 commit into from Jun 6, 2015

Conversation

kropiv
Copy link
Contributor

@kropiv kropiv commented Jun 4, 2015

Dear CMSSW experts,

I would like to request to change primaryVertexCut for displaced Vertex reconstruction from 2.5
to 1.8 cm to observe beam pipe at RUN II in Displaced Vertex collection, because it was changed since RUN I and now for RUN II beam pipe at 2.25 cm. We need this change to perform measurements of beam pipe position in detector.
We have simulated beam pipe with mc_run2 configuration and with new 1.8 cm cut and could reconstruct it in MC (picture is attached).

Best regards, Anna

beampipeforrunii

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 4, 2015

A new Pull Request was created by @kropiv (Anna Kropivnitskaya) for CMSSW_7_5_X.

change primaryVertexCut for displaced Vertex reconstruction from 2.5

It involves the following packages:

RecoParticleFlow/PFTracking

@cmsbuild, @cvuosalo, @slava77 can you please review it and eventually sign? Thanks.
@mmarionncern, @bachtis, @lgray 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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@slava77
Copy link
Contributor

slava77 commented Jun 4, 2015

We still support run1 data and simulation processing.
So, if this change is run2-specific, it should be added in customiseReco
https://github.com/cms-sw/cmssw/blob/CMSSW_7_4_X/SLHCUpgradeSimulations/Configuration/python/postLS1Customs.py#L711
A corresponding era update is needed as well (see e.g.,

eras.run2_common.toModify( particleFlowRecHitHO, func=_modifyParticleFlowRecHitHOForRun2 )
)

@ianna do you happen to have geometry descriptions to compare?

@kropiv
Copy link
Contributor Author

kropiv commented Jun 4, 2015

Dear Slava,

yes, this change is necessary for run2 configuration.
Do I understand correct from your message then I have to request pull request for CMSSW_7_4_X right now and latter it will move it to CMSSW_7_5_X automatically when this version begin support run2?
Could you please confirm that I have to modify this file:
https://github.com/cms-sw/cmssw/blob/CMSSW_7_4_X/SLHCUpgradeSimulations/Configuration/python/postLS1Customs.py#L711
and add new cut here, but not in the place, which I did:
https://github.com/cms-sw/cmssw/blob/CMSSW_7_5_X/RecoParticleFlow/PFTracking/python/particleFlowDisplacedVertex_cfi.py

What you mean about geometry description to compare? Do you want that I add a beam-pipe position for run1?

Thank you in advance, Anna

@slava77
Copy link
Contributor

slava77 commented Jun 4, 2015

Dear Anna

On 6/4/15 10:38 AM, Anna Kropivnitskaya wrote:

Dear Slava,

yes, this change is necessary for run2 configuration.
Do I understand correct from your message then I have to request pull
request for CMSSW_7_4_X right now and latter it will move it to
CMSSW_7_5_X automatically when this version begin support run2?
Could you please confirm that I have to modify this file:
https://github.com/cms-sw/cmssw/blob/CMSSW_7_4_X/SLHCUpgradeSimulations/Configuration/python/postLS1Customs.py#L711
and add new cut here, but not in the place, which I did:
https://github.com/cms-sw/cmssw/blob/CMSSW_7_5_X/RecoParticleFlow/PFTracking/python/particleFlowDisplacedVertex_cfi.py

My links to 74X shouldn't be interpreted as a request for a PR in 74X.
I just used the first that popped up in my browser history to show the
location in the postLS1Customs file to be changed.

I hope we can stay just in 75X without a further request for 74X
(even 75X is already closed for new features, only essential bug fixes
are allowed, but this can probably go in.)

What you mean about geometry description to compare? Do you want that I
add a beam-pipe position for run1?

This was a question to Yana.

Cheers

    --slava

Thank you in advance, Anna


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

@kropiv
Copy link
Contributor Author

kropiv commented Jun 4, 2015

Dear Slava,

thank you for your reply. As I understand I don't need to make anything for a time being. Please let me know if you will need something from me.

As I understand the idea of Tracker conveners (Vincenzo Innocente and Marco Rovere) was to submit this change to CMSSW_7_5_X and after move it to CMSSW_7_4_X (measurement of beam pipe position and nuclear interaction study, which we are also doing, should be done on the data with low luminosity to avoid big pile up). Will it will be possible to move it to CMSSW_7_4_X later? It is very small change.

Best regards, Anna

@slava77
Copy link
Contributor

slava77 commented Jun 4, 2015

Dear Anna,

You need to implement your change differently from the current state of the PR:
You will need to apply your change in postLS1Customs and also add the run2 era related lines in RecoParticleFlow/PFTracking/python/particleFlowDisplacedVertex_cfi.py

At the moment, I think that for 74X you (or who needs this info) will need to rerun manually using RECO inputs.
If this is unacceptable, we will need to discuss another solution (probably in a HN or/and a meeting, not on the github thread).

@VinInn
Copy link
Contributor

VinInn commented Jun 4, 2015

@slava77
Let's if there is any difference in "physics".
If none or negligible we can even leave it as default.
We failed to get an answer from the involved parties since two weeks.
If nobody but us cares of the value in question I do not see why we shall go through the complex process of backward compatibility.

btw rerun manually using RECO inputs is not really an option.
It has to go in Express and Prompt asap

@VinInn
Copy link
Contributor

VinInn commented Jun 4, 2015

@cmsbuild please test

@slava77
Copy link
Contributor

slava77 commented Jun 4, 2015

On 6/4/15 11:45 AM, Vincenzo Innocente wrote:

@slava77 https://github.com/slava77
Let's if there is any difference in "physics".
If none or negligible we can even leave it as default.
We failed to get an answer from the involved parties since two weeks.
If nobody but us cares of the value in question I do not see why we
shall go through the complex process of backward compatibility.

btw rerun manually using RECO inputs is not really an option.
It has to go in Express and Prompt asap

Vincenzo,

I'm not sure I understand the urgency and the requirement to be already
in Express.
Is this data needed in PCL?

Why is rerunning on RECO not an option?
This task looks like it needs ~1M event processed with just PF rereco.
This is faster than making miniAOD.


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

@VinInn
Copy link
Contributor

VinInn commented Jun 4, 2015

On 4 Jun, 2015, at 6:53 PM, Slava Krutelyov notifications@github.com wrote:

Why is rerunning on RECO not an option?
This task looks like it needs ~1M event processed with just PF rereco.
This is faster than making miniAOD.

indeed, I was thinking that it needed rereco of tracking.
It is an option.
Anna et al, please start considering this option.
Please prepare and test a minimal workflow that do re-reco from RECO .

@davidlange6
Copy link
Contributor

On Jun 4, 2015, at 5:17 PM, Slava Krutelyov notifications@github.com wrote:

We still support run1 data and simulation processing.
So, if this change is run2-specific, it should be added in customiseReco
https://github.com/cms-sw/cmssw/blob/CMSSW_7_4_X/SLHCUpgradeSimulations/Configuration/python/postLS1Customs.py#L711
A corresponding era update is needed as well (see e.g.,

eras.run2_common.toModify( particleFlowRecHitHO, func=_modifyParticleFlowRecHitHOForRun2 )
)

@ianna do you happen to have geometry descriptions to compare?

even better - can we derive this value from the geometry itself? It could save us from re-disocvering this in phase 2 after years of being missed in MC studies.


Reply to this email directly or view it on GitHub.

@slava77
Copy link
Contributor

slava77 commented Jun 4, 2015

the discussion digressed a bit into 74X branch implementation.

Lets get this PR right in 75X.

@slava77
Copy link
Contributor

slava77 commented Jun 5, 2015

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2015

The tests are being triggered in jenkins.

@kropiv
Copy link
Contributor Author

kropiv commented Jun 5, 2015

@slava77
Dear Slava,
it is 1st time when I try to update CMSSW. Now I am a little bit confused, what I have to do right now. Will you have time to meet with me or skype, for example at Monday at CERN, that next time it will be more smooth going for me.
Best regards, Anna

@kropiv
Copy link
Contributor Author

kropiv commented Jun 5, 2015

Could you please tell exact command which you run and CMSSW version to perform this test. I just want to be sure that this error come from my change. Also may be I could increase cut to 1.9 or even 2.0 to test if this error will dissipated.

Best regards, Anna

@slava77
Copy link
Contributor

slava77 commented Jun 5, 2015

@kropiv you can ignore the test error, it's not from your code changes

@slava77
Copy link
Contributor

slava77 commented Jun 5, 2015

+1

for #9454 0c0d949

  • a legitimate bugfix with generally fairly small downstream effects
  • tested locally with extended statistics tests included in CMSSW_7_5_0_pre5 /test area sign561; cherry-picked/
  • workflows with pileup are the most affected: 25202 (ttbar with PU=35 at 25 ns) has about 20% more particleFlowDisplacedVertex vertexes
  • time increase is only 12% in the corresponding module
   +0.123295      +0.03%       178.87 ms/ev ->       202.37 ms/ev particleFlowDisplacedVertex
  • changes in the pf objects downstream of particleFlowDisplacedVertex are fairly tiny and none were worth a picture posted here (values shift in a few bins here and there in pfCandidates and further downstream)

Based on this assessment of impact for common objects, it should be safe to include in 74X, but this better go through the regular channels (PPD meeting)

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 5, 2015

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_5_X IBs unless changes (but tests are reportedly failing). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @Degano, @smuzaffar

davidlange6 added a commit that referenced this pull request Jun 6, 2015
change primaryVertexCut for displaced Vertex reconstruction from 2.5 …
@davidlange6 davidlange6 merged commit ff7a105 into cms-sw:CMSSW_7_5_X Jun 6, 2015
@kropiv
Copy link
Contributor Author

kropiv commented Jun 9, 2015

@slava77
Dear Slava,

I wold like to thank you a lot for the merging our changes.
During this week we proceed our study about beam-pipe and we have found that we need also change preliminary algorithm of vertex reconstruction:

https://github.com/cms-sw/cmssw/blob/CMSSW_7_5_X/RecoParticleFlow/PFTracking/python/particleFlowDisplacedVertexCandidate_cfi.py#L20
primaryVertexCut = cms.double(1.8),

before it was 2.2 cm so it slightly effected to our beam-pipe measurement, which is locate around 2.25 cm.
We perform a test for it and you could see it in the attached file. Green curve correspond to reco nuclear interaction with old 2.2 cm cut (preliminary/approximate vertex reconstruction) and blue curve to the new 1.8 cm cut. As you could see with blue we could reconstruct slightly more events with lower radius. We plot radius of nuclear interactions only in the region of beam pipe.

Please let me know how I have to proceed now. Should I request new pull request or it could be done within this one? We need this change to perform not biased measurement of beam pipe and as you could see we again need change only parameter in config file.

I also have question about branching, which I create (NewDisplacedVertexCutMin). Who have to delete it? Me or you?

Thank you in advance, Anna

beampipeforrunii_2

@slava77
Copy link
Contributor

slava77 commented Jun 9, 2015

Dear Anna,

Please create a new pull request with the changes that you need.

    --slava

On 6/9/15 5:02 AM, Anna Kropivnitskaya wrote:

@slava77 https://github.com/slava77
Dear Slava,

I wold like to thank you a lot for the merging our changes.
During this week we proceed our study about beam-pipe and we have found
that we need also change preliminary algorithm of vertex reconstruction:

https://github.com/cms-sw/cmssw/blob/CMSSW_7_5_X/RecoParticleFlow/PFTracking/python/particleFlowDisplacedVertexCandidate_cfi.py#L20
primaryVertexCut = cms.double(1.8),

before it was 2.2 cm so it slightly effected to our beam-pipe
measurement, which is locate around 2.25 cm.
We perform a test for it and you could see it in the attached file.
Green curve correspond to reco nuclear interaction with old 2.2 cm cut
(preliminary/approximate vertex reconstruction) and blue curve to the
new 1.8 cm cut. As you could see with blue we could reconstruct slightly
more events with lower radius. We plot radius of nuclear interactions
only in the region of beam pipe.

Please let me know how I have to proceed now. Should I request new pull
request or it could be done within this one? We need this change to
perform not biased measurement of beam pipe and as you could see we
again need change only parameter in config file.

I also have question about branching, which I create
(NewDisplacedVertexCutMin). Who have to delete it? Me or you?

Thank you in advance, Anna

beampipeforrunii_2
https://cloud.githubusercontent.com/assets/5188748/8055235/46d41ec4-0e9e-11e5-866c-2f579712b5b7.png


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

@kropiv
Copy link
Contributor Author

kropiv commented Jun 9, 2015

Dear Slava,

thank you a lot for your help.

Best, Anna

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

5 participants