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

Hgc deadzone mitigation #5585

Merged
merged 4 commits into from Sep 29, 2014

Conversation

pfs
Copy link
Contributor

@pfs pfs commented Sep 26, 2014

This includes some further protections for invalid cells set by HGCalDDDConstants, following the tests from Fedor.
It also extends the evaluation of the boundaries of the cells to mitigate further the dead zone in the active material.
@vandreev11 @bsunanda @lgray please follow this one as well

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @pfs (Pedro Silva) for CMSSW_6_2_X_SLHC.

Hgc deadzone mitigation

It involves the following packages:

Geometry/FCalGeometry
Geometry/HGCalCommonData
SimG4CMS/Calo

The following packages do not have a category, yet:

Geometry/FCalGeometry

@civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @nclopezo, @ktf can you please review it and eventually sign? Thanks.
@ghellwig 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.

@fratnikov
Copy link

  • virtual uint32_t getUnitID(ForwardSubdetector &subdet, int &layer, int &module, int &iz, const G4ThreeVector &pos);

Why first four parameters, which are essentially not modified integers, are passed by reference here?

@pfs
Copy link
Contributor Author

pfs commented Sep 26, 2014

@fratnikov I don't know exactly - i guess historical (@bsunanda?), but I agree it doesn't make sense. I removed it.

@cmsbuild
Copy link
Contributor

Pull request #5585 was updated. @civanch, @Dr15Jones, @ianna, @mdhildreth, @cmsbuild, @nclopezo, @ktf can you please check and sign again.

@fratnikov
Copy link

Below is the summary of the geometry closure test.
It doesn't look like deadzones are reduced.
============= Geometry Test Summary ============

detector all points no SimId no RecoId no close cell unmatched Id
HGCalEE 334800 2.47e-02 0.00e+00 3.88e-05 2.21e-04
---------- ------------ ---------- ----------- --------------- --------------
HGCalHEF 129600 2.51e-02 0.00e+00 5.40e-05 3.40e-04
---------- ------------ ---------- ----------- --------------- --------------
HGCalHEB 72000 2.99e-02 0.00e+00 0.00e+00 1.39e-05
---------- ------------ ---------- ----------- --------------- --------------

@pfs
Copy link
Contributor Author

pfs commented Sep 26, 2014

@fratnikov interesting i was expecting a decrease of the dead zones but indeed not 0% dead zones
v4 geometry http://psilva.web.cern.ch/psilva/HGCal/Geometry/v4/
In fact I'm not so confident of pushing the cells further to the boundaries, than
we start having cells with significantly less than 50% of their intended size.
I'll check further but in any case it's good to see the numbers in remaining columns :)

@fratnikov
Copy link

Are you going to resolve the dead zone issue before we go ahead with this
PR?
On Sep 26, 2014 1:36 PM, "Pedro Silva" notifications@github.com wrote:

@fratnikov interesting i was expecting a decrease of the dead zones but
indeed not 0% dead zones
v4 geometry http://psilva.web.cern.ch/psilva/HGCal/Geometry/v4/
i'll check further
but in any case it's good to see the numbers in remaining columns :)


Reply to this email directly or view it on GitHub.

@pfs
Copy link
Contributor Author

pfs commented Sep 27, 2014

I'll have stable internet connection only Monday. The test I need to do can be run early morning.
The dead zone will never be 0% nevertheless, but I think i can squeeze it a little bit more.
If not possible, i think we should still go ahead as there are some extra protections which have also been included in this PR.

@fratnikov
Copy link

What we have right now is essentially the same as we had before. I'd expect
deadzone reduction to the order of 10e-3.
On Sep 27, 2014 3:56 PM, "Pedro Silva" notifications@github.com wrote:

I'll have stable internet connection only Monday. The test I need to do
can be run early morning.
The dead zone will never be 0% nevertheless, but I think i can squeeze it
a little bit more.
If not possible, i think we should still go ahead as there are some extra
protections which have also been included in this PR.


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

@vandreev11
Copy link
Contributor

Deadzone size is not the priority of the current release scheduled for Monday.
The issue can be polished later.

On Sep 27, 2014, at 11:35 PM, Fedor Ratnikov wrote:

What we have right now is essentially the same as we had before. I'd expect
deadzone reduction to the order of 10e-3.
On Sep 27, 2014 3:56 PM, "Pedro Silva" notifications@github.com wrote:

I'll have stable internet connection only Monday. The test I need to do
can be run early morning.
The dead zone will never be 0% nevertheless, but I think i can squeeze it
a little bit more.
If not possible, i think we should still go ahead as there are some extra
protections which have also been included in this PR.


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


Reply to this email directly or view it on GitHub.

@fratnikov
Copy link

Is 3-cells offset in v5 geometry tests an issue for the release?

On Sep 28, 2014 1:52 AM, "vandreev11" notifications@github.com wrote:

Deadzone size is not the priority of the current release scheduled for
Monday.
The issue can be polished later.

On Sep 27, 2014, at 11:35 PM, Fedor Ratnikov wrote:

What we have right now is essentially the same as we had before. I'd
expect
deadzone reduction to the order of 10e-3.
On Sep 27, 2014 3:56 PM, "Pedro Silva" notifications@github.com
wrote:

I'll have stable internet connection only Monday. The test I need to
do
can be run early morning.
The dead zone will never be 0% nevertheless, but I think i can
squeeze it
a little bit more.
If not possible, i think we should still go ahead as there are some
extra
protections which have also been included in this PR.


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


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub.

@vandreev11
Copy link
Contributor

All right, we go for the SLHC18 release with all the fixes up to date. We need a trial
production with the current state of art. 4% effect in HEB is tolerable.
There is some homework for the next release.

On Sep 28, 2014, at 4:52 PM, Fedor Ratnikov wrote:

Is 3-cells offset in v5 geometry tests an issue for the release?

On Sep 28, 2014 1:52 AM, "vandreev11" notifications@github.com wrote:

Deadzone size is not the priority of the current release scheduled for
Monday.
The issue can be polished later.

On Sep 27, 2014, at 11:35 PM, Fedor Ratnikov wrote:

What we have right now is essentially the same as we had before. I'd
expect
deadzone reduction to the order of 10e-3.
On Sep 27, 2014 3:56 PM, "Pedro Silva" notifications@github.com
wrote:

I'll have stable internet connection only Monday. The test I need to
do
can be run early morning.
The dead zone will never be 0% nevertheless, but I think i can
squeeze it
a little bit more.
If not possible, i think we should still go ahead as there are some
extra
protections which have also been included in this PR.


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


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub.

@fratnikov
Copy link

merge
doesn't break tests. Consider OK with caveats above

cmsbuild added a commit that referenced this pull request Sep 29, 2014
@cmsbuild cmsbuild merged commit 5ac088c into cms-sw:CMSSW_6_2_X_SLHC Sep 29, 2014
@pfs pfs deleted the hgc_deadzone_mitigation branch November 18, 2021 21:43
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