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

use qScale in pixel cluster charge reweighting #41623

Merged
merged 2 commits into from
May 12, 2023

Conversation

mroguljic
Copy link
Contributor

PR description:

Introducing qScale in the cluster charge reweighting as it is now different from 1 in Run 3. This change is supposed to alleviate the data/mc discrepancy in cluster charge, for example, slide 4 in backup of the following contribution https://indico.cern.ch/event/1271486/contributions/5397175/attachments/2644044/4576218/Pixel_Dynamic_Inefficiency_simulation_Validation___update2.pdf

PR validation:

Run the matrix tests (runTheMatrix.py -l limited -i all --ibeos) all passed:
"47 46 45 35 22 4 1 1 1 1 1 tests passed, 0 0 0 0 0 0 0 0 0 0 0 failed"

Privately ran over ZMM GEN-SIM sample (file /store/relval/CMSSW_12_4_12/RelValZMM_PU_13p6/GEN-SIM/PU_124X_mcRun3_2022_realistic_postEE_v1_PDMVRELVALS188_HS_2023PU-v1/00000/8501a177-d79f-4e60-9ceb-40eac7ec3371.root) and the changes in cluster charge are as expected (red is without change, blue is with the change)
image

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41623/35509

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-41623/35510

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mroguljic (Matej Roguljic) for master.

It involves the following packages:

  • SimTracker/Common (simulation)

@cmsbuild, @civanch, @mdhildreth can you please review it and eventually sign? Thanks.
@JanFSchulte, @echabert, @VourMa, @robervalwalsh, @GiacomoSguazzoni, @gbenelli, @rovere, @VinInn, @missirol, @prolay, @mtosi, @mmusich, @threus, @dgulhan this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@mmusich
Copy link
Contributor

mmusich commented May 11, 2023

test parameters:

  • workflow = 250202.184

@mmusich
Copy link
Contributor

mmusich commented May 11, 2023

@cmsbuild, please test

@mmusich
Copy link
Contributor

mmusich commented May 11, 2023

@mroguljic you might want to change the PR title to: "use qScale in pixel cluster charge reweighting" for clarity in the release notes.

@mroguljic mroguljic changed the title use qScale in charge reweighting use qScale in pixel cluster charge reweighting May 11, 2023
@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ef9132/32556/summary.html
COMMIT: 8aa7190
CMSSW: CMSSW_13_2_X_2023-05-10-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/41623/32556/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 48 lines to the logs
  • Reco comparison results: 13807 differences found in the comparisons
  • DQMHistoTests: Total files compared: 49
  • DQMHistoTests: Total histograms compared: 3564500
  • DQMHistoTests: Total failures: 62048
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3502429
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.21999999999999997 KiB( 48 files compared)
  • DQMHistoSizes: changed ( 250202.181 ): -0.352 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 250202.184 ): 0.132 KiB SiStrip/MechanicalView
  • Checked 212 log files, 164 edm output root files, 49 DQM output files
  • TriggerResults: no differences found

@civanch
Copy link
Contributor

civanch commented May 11, 2023

+1

@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, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@rappoccio
Copy link
Contributor

This looks like it will have a very strong low level effect that will make it hard to disentangle so we should discuss the strategy to incorporate this. What is the plan there?

@mmusich
Copy link
Contributor

mmusich commented May 11, 2023

This looks like it will have a very strong low level effect that will make it hard to disentangle so we should discuss the strategy to incorporate this. What is the plan there

I guess a good proportion of the changes are due to a different random number sequence used when the digitization code is touched.
The changes at the level of reconstructed tracks are generally small in all workflows.
On top of that there is what I guess is the desidered effect to suppress the charge deposition in pixels, which shows in certain histograms. It would be nice to understand if e.g. the strong decrease of charge in pixel forward (even in run2 wfs) is expected: https://tinyurl.com/2h936kvs

@rappoccio
Copy link
Contributor

This looks like it will have a very strong low level effect that will make it hard to disentangle so we should discuss the strategy to incorporate this. What is the plan there

I guess a good proportion of the changes are due to a different random number sequence used when the digitization code is touched. The changes at the level of reconstructed tracks are generally small in all workflows. On top of that there is what I guess is the desidered effect to suppress the charge deposition in pixels, which shows in certain histograms. It would be nice to understand if e.g. the strong decrease of charge in pixel forward (even in run2 wfs) is expected: https://tinyurl.com/2h936kvs

Yes, I would like to see if PPD+PDMV are okay with the validation plan.

@mmusich
Copy link
Contributor

mmusich commented May 12, 2023

Yes, I would like to see if PPD+PDMV are okay with the validation plan.

I am not sure what would be the suggestion, assuming the changes here are expected (@mroguljic @ferencek please have a look to the DQM bin-by-bin differences ) .
Are you proposing to make a pre-release just for this change? I am not sure the overall magnitude of the change justifies that.

@rappoccio
Copy link
Contributor

Yes, I would like to see if PPD+PDMV are okay with the validation plan.

I am not sure what would be the suggestion, assuming the changes here are expected (@mroguljic @ferencek please have a look to the DQM bin-by-bin differences ) . Are you proposing to make a pre-release just for this change? I am not sure the overall magnitude of the change justifies that.

No, I would just like to ensure that they are happy with the plan.

@rappoccio
Copy link
Contributor

+1

Discussing offline with PPD, they are happy with the plan.

@cmsbuild cmsbuild merged commit 768b83c into cms-sw:master May 12, 2023
@ferencek
Copy link
Contributor

ferencek commented May 12, 2023

This looks like it will have a very strong low level effect that will make it hard to disentangle so we should discuss the strategy to incorporate this. What is the plan there

I guess a good proportion of the changes are due to a different random number sequence used when the digitization code is touched. The changes at the level of reconstructed tracks are generally small in all workflows. On top of that there is what I guess is the desidered effect to suppress the charge deposition in pixels, which shows in certain histograms. It would be nice to understand if e.g. the strong decrease of charge in pixel forward (even in run2 wfs) is expected: https://tinyurl.com/2h936kvs

Apologies for the slow response. The changes indeed go in the expected direction. For Run 2, though, we have to double check but based on the output it looks like the Run 2 FPix templates already had qScale significantly different from 1.

@ferencek
Copy link
Contributor

I can confirm that qScale for FPix in Run 2 is indeed significantly different from 1
image

For Run 3 this is the case for both BPix and FPix
image

which is what prompted this PR. So in summary, all changes are as expected.

@tvami
Copy link
Contributor

tvami commented May 17, 2023

hi @mroguljic I think this should be backported to the release that's used in the 2022/2023 MC production (i.e. 13_1_X)

@tvami
Copy link
Contributor

tvami commented May 17, 2023

I was chatting with Matej, we agreed that I can do the backport

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.

7 participants