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.

Sign up for GitHub

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

Jump to bottom

Fix HGCAL Geometry - Revert norot back to False in locateCell() #45782

Merged
merged 1 commit into from
Aug 25, 2024
Merged

Fix HGCAL Geometry - Revert norot back to False in locateCell() #45782

merged 1 commit into from
Aug 25, 2024

Conversation

waredjeb
Copy link
Contributor

This PR aims to fix a problem introduce with #45366, where the norot value in one call of locateCell has been flipped to True (thanks @fwyzard for spotting it).
This caused some problems in the correct position of the DetId on some Layers of HGCAL. We noticed the problem by inspecting some events using Fireworks.

Visualizing events produced with CMSSW_14_1_0_pre6 with the D99 geometry, with a dump of the Reco geometry done using CMSSW_14_1_0_pre1 (currently impossible to dump the geometry in CMSSW_14_1_0_pre6) we noticed some strange behaviour in the TICL Tracksters reconstruction. In [1] you can see how some of the LayerCluster are rotated with respect to the position of the Rechits.
Inspecting in more details the problem we decided to dump the position of one of these DetIds in the different releases

14_1_0_pre6: DetId 2485782895 position  (-63.1768,-51.3877,374.281)  (eta,phi) 2.22995 -2.45874 
14_1_0_pre1: DetId 2485782895 position  (-80.4066,-12.9146,374.281)  (eta,phi) 2.22995 -2.98234 
14_1_0_pre5: DetId 2485782895 position (-80.4066,-12.9146,374.281)  (eta,phi) 2.22995 -2.98234
Fix_14_1_0_pre6: DetId 2485782895  (-80.4066,-12.9146,374.281)  (eta,phi) 2.22995 -2.98234

You can clearly see that in 14_1_0_pre6 the detId is rotated by 30 degrees. This is fixed with this PR as you can see in Fix_14_1_0_pre6 . In [2] you can see the event display after the fix

[1]
image_2024-08-23_12-08-58

[2]
image

@felicepantaleo FYI

@cmsbuild cmsbuild added this to the CMSSW_14_1_X milestone Aug 23, 2024
@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 23, 2024 •
edited
Loading

cms-bot internal usage

@felicepantaleo
Copy link
Contributor

@cmsbuild please test

@felicepantaleo
Copy link
Contributor

@cms-sw/hgcal-dpg-l2 @bsunanda @cms-sw/upgrade-l2 fyi

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-45782/41509

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @waredjeb for master.

It involves the following packages:

  • Geometry/HGCalGeometry (geometry, upgrade)

@Dr15Jones, @bsunanda, @civanch, @makortel, @mdhildreth, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks.
@bsunanda, @fabiocos, @martinamalberti this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@Dr15Jones
Copy link
Contributor

currently impossible to dump the geometry in CMSSW_14_1_0_pre6

@waredjeb please open a GitHub issue fully explaining the problem.

@Dr15Jones
Copy link
Contributor

@waredjeb I see that an issue is already open, it was just so old I didn't find it ( #43097 )

@felicepantaleo
Copy link
Contributor

type hgcal, geometry, bugfix

@felicepantaleo
Copy link
Contributor

type hgcal

@cmsbuild cmsbuild added the hgcal label Aug 23, 2024
@felicepantaleo
Copy link
Contributor

type bug-fix

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals-INPUT
Size: This PR adds an extra 24KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e26725/41097/summary.html
COMMIT: f6a1154
CMSSW: CMSSW_14_1_X_2024-08-22-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45782/41097/install.sh to create a dev area with all the needed externals and cmssw changes.

  • DAS Queries: The DAS query tests failed, see the summary page for details.

RelVals-INPUT

  • 2024.4050052024.405005_RunMuon02024F_50k/step1_dasquery.log
  • 2024.4110052024.411005_RunParkingSingleMuon02024F_50k/step1_dasquery.log
  • 2024.00612024.0061_RunMuonEG2024B_1M/step1_dasquery.log
Expand to see more relval errors ...

Comparison Summary

Summary:

  • You potentially added 15 lines to the logs
  • Reco comparison results: 4507 differences found in the comparisons
  • DQMHistoTests: Total files compared: 44
  • DQMHistoTests: Total histograms compared: 3328202
  • DQMHistoTests: Total failures: 4852
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3323330
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 43 files compared)
  • Checked 191 log files, 161 edm output root files, 44 DQM output files
  • TriggerResults: found differences in 2 / 42 workflows

@felicepantaleo
Copy link
Contributor

@cmsbuild please test

@bsunanda
Copy link
Contributor

+geometry

@bsunanda
Copy link
Contributor

@srimanob please approve this PR and this is one line fix to the geometry code of HGCal. Let it be integrated fast

bsunanda pushed a commit to bsunanda/cmssw that referenced this pull request Aug 24, 2024
Remove the fix in HGCalGeometry because it is ale\reasy provide in cm…
bcd83aa 
…s-sw#45782
@bsunanda
Copy link
Contributor

@cmsbuild Please test

@srimanob
Copy link
Contributor

+Upgrade

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @mandrenguyen, @sextonkennedy, @antoniovilela, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@mandrenguyen
Copy link
Contributor

urgent

@cmsbuild
Copy link
Contributor

-1

Failed Tests: RelVals-INPUT
Size: This PR adds an extra 12KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e26725/41110/summary.html
COMMIT: f6a1154
CMSSW: CMSSW_14_1_X_2024-08-23-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/45782/41110/install.sh to create a dev area with all the needed externals and cmssw changes.

  • DAS Queries: The DAS query tests failed, see the summary page for details.

RelVals-INPUT

  • 2024.3030052024.303005_RunHcalNZS2024E_50k/step1_dasquery.log
  • 2024.4040052024.404005_RunJetMET02024F_50k/step1_dasquery.log
  • 2024.4110052024.411005_RunParkingSingleMuon02024F_50k/step1_dasquery.log
Expand to see more relval errors ...

Comparison Summary

Summary:

  • You potentially added 14 lines to the logs
  • Reco comparison results: 4531 differences found in the comparisons
  • DQMHistoTests: Total files compared: 44
  • DQMHistoTests: Total histograms compared: 3328202
  • DQMHistoTests: Total failures: 3100
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3325082
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 43 files compared)
  • Checked 191 log files, 161 edm output root files, 44 DQM output files
  • TriggerResults: found differences in 2 / 42 workflows

@mandrenguyen
Copy link
Contributor

@cmsbuild ignore tests-rejected with ib-failure

@mandrenguyen
Copy link
Contributor

+1
Failures are related to input files and are not coming from this PR.

@cmsbuild cmsbuild merged commit 3ad0e57 into cms-sw:master Aug 25, 2024
10 of 11 checks passed
@cmsbuild cmsbuild mentioned this pull request Aug 25, 2024
Merged
youngwan-kim pushed a commit to youngwan-kim/cmssw that referenced this pull request Sep 11, 2024
@youngwan-kim
Remove the fix in HGCalGeometry because it is ale\reasy provide in cm…
3542622 
…s-sw#45782
jyoti299 pushed a commit to jyoti299/cmssw that referenced this pull request Oct 1, 2024
Remove the fix in HGCalGeometry because it is ale\reasy provide in cm…
287b402 
…s-sw#45782
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug-fix code-checks-approved fully-signed geometry-approved hgcal orp-approved tests-approved tests-ib-failure upgrade-approved urgent
Projects
None yet
Milestone
CMSSW_14_1_X
Development

Successfully merging this pull request may close these issues.
7 participants
@waredjeb @cmsbuild @felicepantaleo @Dr15Jones @bsunanda @srimanob @mandrenguyen