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

TimeSlew modelling update #6103

Merged
merged 2 commits into from Oct 30, 2014
Merged

Conversation

abdoulline
Copy link

NB: to have it more adequate to 25ns high-PU simulation. Needs to be included in 73X in sync with "Method 2"
#5954

(1)
Current TimeSlew modelling has been developed/implemented quite some time ago and it relies on the "standalone" signal properties, evaluating the time delay of the signal based on the total charge collected in 4TS. It was OK for low-PU sparse BX simulation , but becomes less adequate for high-PU 25ns regime.
(2)
This update still uses existing "TimeSlew vs full-signal" parametrization, but it makes estimates for "full signal" for each 1TS and delays each TS accordingly, which corresponds better to the hardware operation mode.
Still keeps previous TimeSlew smearing evaluated in 2010 (to mimic real data timing variation).
(3)
More advanced and detailed simulation of the HCAL FE electronics properties is being developed, but out of scope of 730.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @abdoulline for CMSSW_7_3_X.

TimeSlew modelling update

It involves the following packages:

SimCalorimetry/HcalSimAlgos

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

@civanch
Copy link
Contributor

civanch commented Oct 30, 2014

Hi Salavat, in line 52 the vector is not fully initialized. Why not replace lines 51-52 by
std::vector data = samples;

Also it seems that in the line 78 vector is filled out of the vector boundary.

In line 80 it is possible to have
samples = data;

@abdoulline
Copy link
Author

Thank you, Vladimir, for spotting those issues, sorry abut that ("Monday 

moning implementation" then left "abandoned"...).
Update/fix committed.
S.

On Thu, 30 Oct 2014, Vladimir Ivantchenko wrote:

Hi Salavat, in line 52 the vector is not fully initialized. Why not replace lines
51-52 by
std::vector data = samples;

Also it seems that in the line 78 vector is filled out of the vector boundary.

In line 80 it is possible to have
samples = data;


Reply to this email directly or view it on
GitHub.[AEx02qIYkbuw7oOxQ6dIuYPukv-BiMQrks5nIgWRgaJpZM4C0yVi.gif]

@cmsbuild
Copy link
Contributor

Pull request #6103 was updated. @cmsbuild, @civanch, @nclopezo, @mdhildreth can you please check and sign again.

@slava77
Copy link
Contributor

slava77 commented Oct 30, 2014

Hi Salavat,
Could you please post a link (or plots) in the PR description describing the physics level of changes expected.

Thanks

@abdoulline
Copy link
Author

The chages aren't expected to be dramatic (several %), being 

(privately) evaluated now.
I presume noPU RelVals is should be good enough (automatically generated
comparison by git as well).
We need to consider it together with Method 2 implementation at the end...

On Thu, 30 Oct 2014, Slava Krutelyov wrote:

Hi Salavat,
Could you please post a link (or plots) in the PR description describing the physics level of
changes expected.

Thanks


Reply to this email directly or view it on
GitHub.[AEx02iycOfDUA6xJibwdr3JRlrC1IEYWks5nIiYdgaJpZM4C0yVi.gif]

@cmsbuild
Copy link
Contributor

@abdoulline
Copy link
Author

For e=50 GeV pions ECAL+HCAL scan there is 5-6% scale reduction "out of the box" (~10% in HCAL) with Method 1 (current default).

https://cms-cpt-software.web.cern.ch/cms-cpt-software/General/Validation/SVSuite/HCAL/calo_scan_single_pi/730pre1_newTimeSlew_vs_730pre1_SinglePi/

To correct for it, better to have this Digitization update coupled with Method 2

@civanch
Copy link
Contributor

civanch commented Oct 30, 2014

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_3_X IBs unless changes (tests are also fine). This pull request will be automatically merged.

cmsbuild added a commit that referenced this pull request Oct 30, 2014
@cmsbuild cmsbuild merged commit df7c27e into cms-sw:CMSSW_7_3_X Oct 30, 2014
@abdoulline abdoulline deleted the TimeSlewSim_update branch March 19, 2015 07:29
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

4 participants