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

fix peak mode SiStrip noise payload in 2018 cosmics simulation #41120

Merged
merged 1 commit into from
Mar 23, 2023

Conversation

mmusich
Copy link
Contributor

@mmusich mmusich commented Mar 21, 2023

resolves #41108

PR description:

In DBG IBs, the function SiStripNoises::getNoise verifies that the strip for which the noise is asked lies in the SiStripNoises::Range of that DetId:

static float getNoise(uint16_t strip, const Range& range) {
#ifdef EDM_ML_DEBUG
verify(strip, range);
#endif
return getNoiseFast(strip, range);
}

definition of SiStripNoises::verify

void SiStripNoises::verify(uint16_t strip, const Range& range) {
if (9 * strip >= (range.second - range.first) * 8)
throw cms::Exception("CorruptedData")
<< "[SiStripNoises::getNoise] looking for SiStripNoises for a strip out of range: strip " << strip;
}

if that's not verified, the code will throw an exception.
In normal builds if that doesn't happen, there is no exception thrown, though of course the used event setup data might be wrong.
This PR proposes a new peak mode noise payload that ensures that all per-strip noise data complies with the verify requirement.
@cms-sw/alca-l2 I propose here a candidate, please let me know if you prefer to transform it upfront into a full-fledged Global Tag, I can then subsequently pick it up here.

PR validation:

run successfully runTheMatrix.py -l 7.4

For reference here's a comparison of the bugged payload (SiStripNoise_PeakMode_2018_Minus20C_v0_mc) vs the fixed one (SiStripNoise_PeakMode_2018_Minus20C_v0_mc_fixed)

image

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:

N/A

@mmusich
Copy link
Contributor Author

mmusich commented Mar 21, 2023

type bug-fix

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41120/34759

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @mmusich (Marco Musich) for master.

It involves the following packages:

  • Configuration/AlCa (alca)

@cmsbuild, @tvami, @saumyaphor4252, @francescobrivio can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @missirol, @mmusich, @fabiocos, @tocheng 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 Author

mmusich commented Mar 21, 2023

@cmsbuild, please test for CMSSW_13_1_DBG_X

@mmusich mmusich changed the title fix peak mode noise payload in 2018 cosmics simulation fix peak mode SiStrip noise payload in 2018 cosmics simulation Mar 21, 2023
@mmusich
Copy link
Contributor Author

mmusich commented Mar 22, 2023

is it normal that running the DBG relvals takes so long?

@mmusich
Copy link
Contributor Author

mmusich commented Mar 22, 2023

@cmsbuild, please abort

@mmusich
Copy link
Contributor Author

mmusich commented Mar 22, 2023

@cmsbuild, please test workflow 7.4

@tvami
Copy link
Contributor

tvami commented Mar 22, 2023

is it normal that running the DBG relvals takes so long?

@smuzaffar ?

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e90a04/31506/summary.html
COMMIT: 3e98439
CMSSW: CMSSW_13_1_X_2023-03-21-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41120/31506/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 18 lines to the logs
  • Reco comparison results: 9 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3585875
  • DQMHistoTests: Total failures: 9
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3585844
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 218 log files, 167 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@tvami
Copy link
Contributor

tvami commented Mar 22, 2023

@cmsbuild, please test workflow 7.4 for CMSSW_13_1_DBG_X

@mmusich
Copy link
Contributor Author

mmusich commented Mar 22, 2023

please test workflow 7.4 for CMSSW_13_1_DBG_X

I don't see how this is going to help (I see it re-building everything from scratch again).

@tvami
Copy link
Contributor

tvami commented Mar 22, 2023

I was just hoping the issue with the DBG build was temporary. Anyway, Marco, I cut a new real GT for you 131X_upgrade2018cosmics_realistic_peak_v2

@tvami
Copy link
Contributor

tvami commented Mar 22, 2023

Also, will you submit a PR that has the code that ensures that all per-strip noise data complies with the verify requirement?

@mmusich
Copy link
Contributor Author

mmusich commented Mar 22, 2023

Also, will you submit a PR that has the code that ensures that all per-strip noise data complies with the verify requirement?

sure, see #41127

@smuzaffar
Copy link
Contributor

is it normal that running the DBG relvals takes so long?

yes , runtime for DBG is 2/3 time long so yes it can take a lot time. Also as we do not build DBG IB every day so you might pick up an old build (few days old) and then bot might have to rebuild a lot stuff

@mmusich
Copy link
Contributor Author

mmusich commented Mar 22, 2023

I cut a new real GT for you 131X_upgrade2018cosmics_realistic_peak_v2

thanks, it's included now in this PR.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41120/34772

  • This PR adds an extra 12KB to repository

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

Pull request #41120 was updated. @cmsbuild, @tvami, @saumyaphor4252, @francescobrivio can you please check and sign again.

@tvami
Copy link
Contributor

tvami commented Mar 22, 2023

@cmsbuild, please test workflow 7.4 for CMSSW_13_1_DBG_X

  • and I guess we need to be patient with it

@mmusich
Copy link
Contributor Author

mmusich commented Mar 22, 2023

and I guess we need to be patient with it

I guess you can also locally test and sign...

@tvami
Copy link
Contributor

tvami commented Mar 22, 2023

+alca

  • tested locally with the changes

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

@mmusich
Copy link
Contributor Author

mmusich commented Mar 23, 2023

Screenshot from 2023-03-23 15-41-18

Waiting for what?

@tvami
Copy link
Contributor

tvami commented Mar 23, 2023

@cms-sw/orp-l2 can we merge this based on #41120 (comment) and that the std CMSSW test did pass #41120 (comment) ?

@perrotta
Copy link
Contributor

@cmsbuild, please test workflow 7.4
(let have at least a round of "normal" bot tests run after the last update)

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e90a04/31565/summary.html
COMMIT: e4a92e9
CMSSW: CMSSW_13_1_X_2023-03-22-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41120/31565/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 9 lines to the logs
  • Reco comparison results: 9 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3585875
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3585847
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 218 log files, 167 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit d16a1cc into cms-sw:master Mar 23, 2023
@mmusich mmusich deleted the fixPeakModeCosmicSimu_in_DBG_X_IBs branch March 23, 2023 18:44
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.

[DBG] Exception of category CorruptedData in RelVal wf 7.4
5 participants