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

Phase2-hgx323 Further correction for ring number assignment of scintillator hits in HGCal geometry versions beyond V13 #39164

Merged
merged 2 commits into from Aug 25, 2022

Conversation

bsunanda
Copy link
Contributor

@bsunanda bsunanda commented Aug 24, 2022

PR description:

Further correction for ring number assignment of scintillator hits in HGCal geometry versions V14, V15, V16 and V17

PR validation:

Use the 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:

This is a bug fix for all versions of HGCal geometry

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39164/31749

  • This PR adds an extra 48KB to repository

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

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39164/31750

  • This PR adds an extra 52KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • Geometry/CaloTopology (geometry)
  • Geometry/HGCalCommonData (geometry, upgrade)
  • SimG4CMS/Calo (simulation)

@civanch, @Dr15Jones, @bsunanda, @makortel, @ianna, @mdhildreth, @cmsbuild, @AdrianoDee, @srimanob can you please review it and eventually sign? Thanks.
@felicepantaleo, @rovere, @beaucero, @trtomei, @thomreis, @simonepigazzini, @fabiocos, @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
Copy link
Contributor Author

@cmsbuild Please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9f1384/27040/summary.html
COMMIT: 034bf8d
CMSSW: CMSSW_12_5_X_2022-08-23-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39164/27040/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 47146 lines to the logs
  • Reco comparison results: 2817 differences found in the comparisons
  • DQMHistoTests: Total files compared: 51
  • DQMHistoTests: Total histograms compared: 3693084
  • DQMHistoTests: Total failures: 37996
  • DQMHistoTests: Total nulls: 5
  • DQMHistoTests: Total successes: 3655061
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 50 files compared)
  • DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 212 log files, 49 edm output root files, 51 DQM output files
  • TriggerResults: no differences found

@srimanob
Copy link
Contributor

type bug-fix

@srimanob
Copy link
Contributor

Hi @bsunanda
Could you please elaborate more detail of the bug and this fix? This will help us to understand the situation more clearly.

  • Does the bug come after 12_4_0_pre4 as discussed in validation result, or it was there before?
  • If it was there before, how it is found? Do we have any kind of DQM that can spot on this? So that when new relvals come, validator can check on it.

Thanks.

@bsunanda
Copy link
Contributor Author

This bug was there all the time. When the position at the border of 2 rings, it is usually referred to as the lower ring number. This is fine as long as there is no tile in the lower ring. We did not have a good valid test of a detid earlier. We have to introduce one now and that revealed the discrepancy. We do have detailed testing code in the repository and we do conduct them to look into details. The standard validation code can look into broad discrepancies so far and the detailed tests are better done by the experts of the geometry code.

@civanch
Copy link
Contributor

civanch commented Aug 25, 2022

+1

@srimanob
Copy link
Contributor

+Upgrade

The fix should be followed up in validation.

@cmsbuild
Copy link
Contributor

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)

(iphi <= hgpar_->scintCells(layer)));
bool ok = ((irad >= hgpar_->iradMinBH_[indx.first]) && (irad <= (hgpar_->iradMaxBH_[indx.first] + 1)) && (iphi > 0) &&
(iphi <= hgpar_->scintCells(layer)));
return ((ok && trapezoidFile()) ? tileExist(zside, layer, irad, iphi) : ok);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be correct, it should be

Suggested change
return ((ok && trapezoidFile()) ? tileExist(zside, layer, irad, iphi) : ok);
return ((ok && trapezoidFile()) ? tileExist(zside, layer, irad, iphi) : true);

(even if by now the net result is the same)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think your correction is right - irrespective of trapezoidFile() the earlier test is good for all types of scintillators

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think your correction is right - irrespective of trapezoidFile() the earlier test is good for all types of scintillators

I understand that in practice ok && ok is identical to ok && true. But translating in C++ the sentence "irrespective of trapezoidFile() the earlier test is good for all types of scintillators" is exactly ok && true

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 60996e5 into cms-sw:master Aug 25, 2022
@srimanob
Copy link
Contributor

By the way, I see failure in Static Check build log,
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9f1384/27040/llvm-analysis/runStaticChecks.log

Sorry that I miss to look on it when approved the PR.

@srimanob
Copy link
Contributor

srimanob commented Aug 25, 2022

+1

Hmm ... D77 is not expected for the change. There is nothing in PR mentioned about the effect on old geometry. I think this question should be answered first. I am very sorry to miss this when review. I mostly focused on D88 in private production and forget to look on PR test result.

@perrotta
Copy link
Contributor

perrotta commented Aug 25, 2022

Indeed @srimanob , we were too quick

Let revert this, so that it won't pollute pre5: #39198

@bsunanda please redo the PR, and fix it by taking #39164 (comment) and #39164 (comment) into account (and also #39164 (review))

@bsunanda
Copy link
Contributor Author

D77 is expected for the change - I mentioned V16 and V17 because they are to be kept for future. It will certainly affect V14 and V15 as well. D77 is V14

@bsunanda
Copy link
Contributor Author

Please do not revert it. The static errors in topology is for debug. I shall take care of that as well as for HCAL stuff separately.

@bsunanda bsunanda changed the title Phase2-hgx323 Further correction for ring number assignment of scintillator hits in HGCal geometry versions V16 and V17 Phase2-hgx323 Further correction for ring number assignment of scintillator hits in HGCal geometry versions beyond V13 Aug 26, 2022
@srimanob
Copy link
Contributor

Hi @bsunanda
Thanks for clarification. Please open the new PR.

@bsunanda
Copy link
Contributor Author

bsunanda commented Aug 26, 2022 via email

@srimanob
Copy link
Contributor

I leave the technical part to @cms-sw/orp-l2 since this PR is merged already. I don't know if it is possible to do.

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