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

Truncate pt of PackedCandidate #27988

Merged
merged 3 commits into from
Sep 20, 2019
Merged

Truncate pt of PackedCandidate #27988

merged 3 commits into from
Sep 20, 2019

Conversation

rappoccio
Copy link
Contributor

PR description:

In response to #27374, truncating pt of PackedCandidate in case the 32 bit representation is beyond the 16-bit max (represented by the "minifloat" as inf, as per that standard requirement here). This was giving problems downstream when the 16-bit "infinity" was widened to 32 bits and became inaccurate.

PR validation:

This is so deep in the software stack that it is challenging to predict the behavior without ALL of the validations being run.

@rappoccio
Truncate pt of PackedCandidate
d81b42d 
In response to cms-sw#27374, truncating pt of PackedCandidate in case the 32 bit representation is beyond the 16-bit max (represented by the "minifloat" as inf, as per that standard requirement [here](ftp://ftp.fox-toolkit.org/pub/fasthalffloatconversion.pdf)).  This was giving problems downstream when the 16-bit "infinity" was widened to 32 bits and became inaccurate.
@cmsbuild cmsbuild added this to the CMSSW_11_0_X milestone Sep 13, 2019
@rappoccio rappoccio mentioned this pull request Sep 13, 2019
Closed
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@perrotta
Copy link
Contributor

@gpetruc FYI

@perrotta
Copy link
Contributor

Thank you @rappoccio
Could you please run the validation on your sample with problematic events, just to verify that this actually fixes the issue in #27374? Then for a larger scale validation (on "normal" events) we'll rely on the usual RelVals

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27988/11882

  • This PR adds an extra 20KB to repository

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

@rappoccio
Update PackedCandidate.cc
3c41623 
Adding a space.
@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27988/11883

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

DataFormats/PatCandidates

@perrotta, @cmsbuild, @santocch, @slava77 can you please review it and eventually sign? Thanks.
@gouskos, @hatakeyamak, @rovere, @cbernet, @gpetruc, @peruzzim this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Sep 13, 2019 •
edited
Loading

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/2509/console Started: 2019/09/13 16:23

@cmsbuild
Copy link
Contributor

-1

Tested at: 3c41623

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-326d25/2509/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-326d25/2509/git-merge-result

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-326d25/2509/summary.html

I found follow errors while testing this PR

Failed tests: Build ClangBuild

  • Build:

I found compilation error when building:

>> Compiling  /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-09-12-2300/src/DataFormats/PatCandidates/src/Vertexing.cc 
>> Compiling  /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-09-12-2300/src/DataFormats/PatCandidates/src/Electron.cc 
>> Compiling  /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-09-12-2300/src/DataFormats/PatCandidates/src/Muon.cc 
>> Compiling  /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-09-12-2300/src/DataFormats/PatCandidates/src/TauPFSpecific.cc 
/build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-09-12-2300/src/DataFormats/PatCandidates/src/PackedCandidate.cc: In member function 'void pat::PackedCandidate::pack(bool)':
/build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-09-12-2300/src/DataFormats/PatCandidates/src/PackedCandidate.cc:14:74: error: no matching function for call to 'min(ROOT::Math::LorentzVector >::Scalar, float)'
   float unpackedPt = std::min(p4_.load()->Pt(), MiniFloatConverter::max());
                                                                          ^
In file included from /cvmfs/cms-ib.cern.ch/nweek-02593/slc7_amd64_gcc820/external/gcc/8.2.0-pafccj/include/c++/8.3.1/bits/specfun.h:45,
                 from /cvmfs/cms-ib.cern.ch/nweek-02593/slc7_amd64_gcc820/external/gcc/8.2.0-pafccj/include/c++/8.3.1/cmath:1892,
                 from /cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc820/cms/cmssw-patch/CMSSW_11_0_X_2019-09-12-2300/src/DataFormats/Math/interface/deltaPhi.h:11,
  • Clang:

I found compilation error while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'


>> Compiling  /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-09-12-2300/src/DataFormats/PatCandidates/src/Muon.cc 
>> Compiling  /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-09-12-2300/src/DataFormats/PatCandidates/src/PATTauDiscriminator.cc 
>> Compiling  /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-09-12-2300/src/DataFormats/PatCandidates/src/TauPFSpecific.cc 
>> Compiling  /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-09-12-2300/src/DataFormats/PatCandidates/src/TriggerFilter.cc 
>> Compiling  /build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-09-12-2300/src/DataFormats/PatCandidates/src/TauJetCorrFactors.cc 
/build/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_0_X_2019-09-12-2300/src/DataFormats/PatCandidates/src/PackedCandidate.cc:14:22: error: no matching function for call to 'min'
  float unpackedPt = std::min(p4_.load()->Pt(), MiniFloatConverter::max());
                     ^~~~~~~~
/cvmfs/cms-ib.cern.ch/nweek-02593/slc7_amd64_gcc820/external/gcc/8.2.0-pafccj/lib/gcc/x86_64-unknown-linux-gnu/8.3.1/../../../../include/c++/8.3.1/bits/algorithmfwd.h:383:5: note: candidate template ignored: deduced conflicting types for parameter '_Tp' ('double' vs. 'float')
    min(const _Tp&, const _Tp&);
    ^

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-326d25/2509/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-326d25/2509/git-merge-result

@cmsbuild
Copy link
Contributor

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

@rappoccio
Copy link
Contributor Author

I can do a backport if desired, but let me know.

@perrotta
Copy link
Contributor

@rappoccio could you please verify as in #27374 (comment) that the packing of the candidates gets fixed with this PR?

@rappoccio
Copy link
Contributor Author

Apologies for the delay. I confirm that this is now working as intended.

Before fix:

Events->Scan("patPackedCandidates_packedPFCandidates__PAT.obj.pt()", "patPackedCandidates_packedPFCandidates__PAT.obj.pt() > 100");
***********************************
*    Row   * Instance * patPacked *
***********************************
*        0 *     1614 *       inf *
***********************************

After fix:

Events->Scan("patPackedCandidates_packedPFCandidates__PAT.obj.pt()", "patPackedCandidates_packedPFCandidates__PAT.obj.pt() > 100");
***********************************
*    Row   * Instance * patPacked *
***********************************
*        0 *     1614 *     65504 *
***********************************

So this is giving the 16-bit max as intended.

@perrotta
Copy link
Contributor

Thank you @rappoccio : everything as expected, but better to check it explicitly

@perrotta
Copy link
Contributor

+1

  • The fix replaces nan's in miniAOD outputs with some overflow value for nonphysically monster energy jets in PackedCandidate, thus avoiding possible crashes in the code that try to access them.
  • Jenkins tests pass and show no differences for non pathological events

@perrotta
Copy link
Contributor

One has to decide whether it is worth backporting this to 10_6_X

Discussing it with Slava we agreed that this could be considered more a bug fix that prevents unwanted crashes in productions (e.g. nanoAOD), rather than a behavior changing PR. Then probably a backport PR should be foreseen. What do you think, @fabiocos?

@fabiocos
Copy link
Contributor

@perrotta to my mind this is just a protection for pathological situations, so I do not see any violation of the no-physics-change policy if a backport is done (and I would consider it useful as 10_6_X should serve us for long time). So I would suggest to go ahead wit it

@andrzejnovak
Copy link
Contributor

andrzejnovak commented Sep 20, 2019 via email

rappoccio added a commit to rappoccio/cmssw that referenced this pull request Sep 20, 2019
@rappoccio
Update PackedCandidate.cc
0c33dc4 
Backport of changes from cms-sw#27988
rappoccio added a commit to rappoccio/cmssw that referenced this pull request Sep 20, 2019
@rappoccio
Update PackedCandidate.cc
33bf3a3 
Backport of changes from cms-sw#27988
@rappoccio rappoccio mentioned this pull request Sep 20, 2019
Merged
@rappoccio
Copy link
Contributor Author

Backport is #28043

@slava77
Copy link
Contributor

slava77 commented Sep 20, 2019

@perrotta also if you backport this, the boosted sv producer protection should be redundant

it will actually not be redundant for the cases of running on old miniAOD (e.g. some nanoAOD reprocessing with btags remade). I'm not sure though if we have these cases at the production scale for 106X.

@fabiocos
Copy link
Contributor

+1

@fabiocos
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit b891f99 into cms-sw:master Sep 20, 2019
@santocch
Copy link

+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 be automatically merged.

cmsbuild added a commit that referenced this pull request Sep 25, 2019
@cmsbuild
Merge pull request #28043 from rappoccio/patch-4
2574ac3 
Backport of #27988 (Truncate pt of PackedCandidate)
@rappoccio rappoccio deleted the patch-2 branch December 5, 2019 16:33
@slava77 slava77 mentioned this pull request Sep 30, 2020
Merged
@slava77 slava77 mentioned this pull request Oct 20, 2020
Closed
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
analysis-approved code-checks-approved comparison-available fully-signed orp-approved reconstruction-approved tests-approved
Projects
None yet
Milestone
CMSSW_11_0_X
Development

Successfully merging this pull request may close these issues.
7 participants
@rappoccio @cmsbuild @perrotta @slava77 @fabiocos @andrzejnovak @santocch