Phase2-hgx321 Make some fixes to the issues in storing hits for the scintillator part of HGCal

[bsunanda]

PR description:

Make some fixes to the issues in storing hits for the scintillator part of HGCal.

In the process of debugging the SimHits for scintillators, several hit id's were found to be declared to be invalid. This was traced down to wrong assignment of the ID's from position. Most likely while introducing the cassette concept, the conversion between SIM/RECO unit was disturbed. That led to wrong assignment of DetId's. This is cured by this PR. In the process, possible shifts due to cassette positioning etc are rightly handled now. Most of the codes are used for debugging while a very small fraction is used in the fix.

Since it is a bug fix the comparison plots for many phase2 scenarios will look different

PR validation:

Use several phase2 runTheMatrix test workflows

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

Should be backported to 12_4_X if any MC production for Phase2 scenario will be done using that

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39130/31687

• This PR adds an extra 76KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

• code-format:
  https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39130/31687/code-format.patch
  e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39130/31687/code-format.patch | patch -p1
  You can also run scram build code-format to apply code format directly

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39130/31688

• This PR adds an extra 80KB to repository

[cmsbuild]

A new Pull Request was created by @bsunanda (Sunanda Banerjee) for master.

It involves the following packages:

• Geometry/HGCalCommonData (geometry, upgrade)
• Geometry/HGCalGeometry (geometry, upgrade)
• SimG4CMS/Calo (simulation)
• SimG4Core/Application (simulation)

@civanch, @Dr15Jones, @bsunanda, @makortel, @ianna, @mdhildreth, @cmsbuild, @AdrianoDee, @srimanob can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @rovere, @fabiocos, @trtomei, @thomreis, @simonepigazzini, @beaucero, @slomeo this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

[bsunanda]

@cmsbuild Please test

[civanch]

@bsunanda , two questions:

1. is HGCalHitScint an optimal name? It is basically a test, should this be reflected in the name?

2. An extra file is used to handle scintillation hits. How big it is and how it will be used?

[bsunanda]

@civanch There was a complaint of issues for scintillator hits in HGCal like an earlier issue for hits in partial wafers in V17 geometry version. We had to write several test code to resolve that issue. For scintillator hits there. is one test code and may be we can call this HGCalTestScintHits or something like that. This is a test code and will not be used in standard simulation. The changes made in the SD code and the parameter is essentially providing a provision to test for particular set of tiles (as we did for partial wafers) which is useful now and even for future. The issue for scintillator hits have been there for all phase2 versions - noticed in some strange way for the D88 scenario.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39130/31691

• This PR adds an extra 12KB to repository

• Found files with invalid states:

  • SimG4CMS/Calo/plugins/HGCalHitScint.cc:
    • Added: 2cff8cd
    • Modified: abbaf45
    • Deleted: 5729689

[srimanob]

Validation plots can be seen in
https://tinyurl.com/2km9qe7c (HGCAL / HGCalValidator / ticlSimTracksters / TSToCP_linking)
https://tinyurl.com/2kmctz9w (HGCAL / HGCalSimHitsV / HGCalHEScintillatorSensitive)
The sample share the same GEN. Digi and RECO are done as mentioned in the bullets.

Two example plots:

[rovere]

Ciao @bsunanda @srimanob thanks for following up on this.
From private emails and from the plots linked by @srimanob above it seems that everything is back to normal.

For the record and to keep an history of what happened and how it has been fixed, it would be nice to have some further information from Sunanda on this bug and on the fix.

[civanch]

+1

[bsunanda]

It is difficult to know what happened. In the process of debugging the SimHits for scintillators, I found several hit id's are declared to be invalid. This was then traced down to wrong assignment of the ID's from position. Most likely while introducing the cassette concept, the conversion between SIM/RECO unit was disturbed. That led to wrong assignment of DetId's. This is cured. Inthe process, possible shifts due to cassette positioning etc are rightly handled now. Most of the codes are used for debugging while a very small fraction is used in the fix.

[perrotta]

It is difficult to know what happened. In the process of debugging the SimHits for scintillators, I found several hit id's are declared to be invalid. This was then traced down to wrong assignment of the ID's from position. Most likely while introducing the cassette concept, the conversion between SIM/RECO unit was disturbed. That led to wrong assignment of DetId's. This is cured. Inthe process, possible shifts due to cassette positioning etc are rightly handled now. Most of the codes are used for debugging while a very small fraction is used in the fix.

I took the liberty to add this comment to the PR description

[bsunanda]

@srimanob @rovere Could you sign this please? Once this is integrated, we have a few more things to correct.

[srimanob]

Hi @bsunanda

This is what I would like to discuss. Should we wait HGCAL validator to look on DQM first, and report or we merge and produce relvals (as the schedule for _pre5 is tomorrow).

Additional question:
(1) In the 12_5_0_pre3 validation report, it said "inconsistency in 300 um part under 12_5_0_pre2 is recovered in 12_5_0_pre3" What is the PR that fix the issue? And in that fix, what is the result that combines with this PR?

(2) In the PR description, Should be backported to 12_4_X if any MC production for Phase2 scenario will be done using that
What is wrong for 12_4? I assume we use 12_4_0_pre4 as a good reference upto now. What about 12_3 that we run D88 production.

[bsunanda]

There are bug fixes in this PR. I should not wait for validation results for approving this. Validation will come later. If there are still other issues we have to work on them. That is my point of view

[bsunanda]

Since this is a bug report, we prefer the 12_4 version to be rechecked. I know for certain, 12_4_X has holes and some of them can be fixed before starting Phase2 production

[srimanob]

+Upgrade

This PR provides the fix to simhits of HGCAL scintillator part. The validation plot from private production can be found in https://cernbox.cern.ch/index.php/s/cj7pX32FGw7nKam (I expect that the report will be posted to the indico, maybe under HGCAL DPG). The official validation should be done again to confirm the fix.

Note that, this is not the final fix for D88. The silicon sensor part in HE still have a problem in very high eta region (see slide 5 of above link).

[srimanob]

By the way, there will be no Phase-2 production in 12_4. I think we don't need the backport.

[perrotta]

@felicepantaleo,@rovere,@pfs,@cseez this PR is expected to enter pre5, but the HGCal signature is also required.
Could you please have a look and either comment, or sign it?

[rovere]

+1

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

[perrotta]

+1