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

skip empty lines in JetCorrectorParameters ctor #32387

Merged

Conversation

missirol
Copy link
Contributor

@missirol missirol commented Dec 4, 2020

PR description:

While testing with the class JetCorrectorDBWriter, I encountered a seg-fault coming from JetCorrectorParameters.

What led to the crash was the fact that the input .txt file I was using had an empty line in it (afaik, empty lines are absent in standard JEC txt files, thus no crashes).

This PR applies a quick fix, skipping empty lines in JetCorrectorParameters, and making the sw a bit more robust; a more sophisticated fix is left to experts of this package [*].

No changes expected.

FYI: @lathomas @kirschen @juska @glatino @pallabidas @sparedes

[*] The core issue is that the ctor of JetCorrectorParameters::Record does not throw an error if the input string has an invalid format (e.g. empty); in those cases, the mMin/mMax vectors are not filled, and accessing xMin(i) later on here leads to a crash.

PR validation:

Validated with private tests.

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2020

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32387/20253

  • This PR adds an extra 16KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2020

A new Pull Request was created by @missirol (Marino Missiroli) for master.

It involves the following packages:

CondFormats/JetMETObjects

@ggovi, @cmsbuild, @santocch can you please review it and eventually sign? Thanks.
@mmarionncern, @ahinzmann, @rappoccio, @jdamgov, @jdolen, @nhanvtran, @gkasieczka, @clelange, @schoef, @mmusich, @mariadalfonso, @seemasharmafnal this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@santocch
Copy link

santocch commented Dec 4, 2020

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2020

+1
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6514be/11341/summary.html
CMSSW: CMSSW_11_3_X_2020-12-04-1100
SCRAM_ARCH: slc7_amd64_gcc900

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 4, 2020

Comparison results are now available
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-6514be/11341/summary.html
CMSSW: CMSSW_11_3_X_2020-12-04-1100
SCRAM_ARCH: slc7_amd64_gcc900

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 35
  • DQMHistoTests: Total histograms compared: 2529593
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2529564
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
  • Checked 148 log files, 37 edm output root files, 35 DQM output files

@santocch
Copy link

santocch commented Dec 7, 2020

+1

@silviodonato
Copy link
Contributor

merge
@ggovi please let us know if you agree, it seems a trivial fix

@cmsbuild cmsbuild merged commit 691f690 into cms-sw:master Dec 7, 2020
@missirol missirol deleted the devel_jetCorrectorParameters_1120pre10 branch December 7, 2020 18:04
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

4 participants