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

Fixing compiler warnings after updating to fastjet 3.3.0. #22305

Merged
merged 1 commit into from Mar 1, 2018

Conversation

rappoccio
Copy link
Contributor

Responding to #22297, here are a few updates:

  1. Changing to use selectors instead of range definition.
  2. Removing fastjet 2 ghost placement.
  3. Removing unused test-large-voronoi-area.cc.
  4. Removing range definition entirely from the PileUpSubtractor, it is unused.

The second change will induce small but expected changes in the jet areas.

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@slava77
Copy link
Contributor

slava77 commented Feb 22, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 22, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/26239/console Started: 2018/02/22 18:01

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

RecoJets/JetProducers

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@TaiSakuma, @jdolen, @ahinzmann, @jdamgov, @yslai, @nhanvtran, @gkasieczka, @schoef, @mariadalfonso, @seemasharmafnal 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

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22305/26239/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 482 differences found in the comparisons
  • DQMHistoTests: Total files compared: 28
  • DQMHistoTests: Total histograms compared: 2498056
  • DQMHistoTests: Total failures: 4843
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2493037
  • DQMHistoTests: Total skipped: 176
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.01999999998 KiB( 23 files compared)
  • Checked 115 log files, 9 edm output root files, 28 DQM output files

@rappoccio
Copy link
Contributor Author

The comparisons look as expected. There are a few small changes due to the ghost placement change.

@perrotta
Copy link
Contributor

@rappoccio : trying to find some documentation which explains your point 2 I ended up jumping on
http://fastjet.fr/repo/doxygen-3.3.0/classfastjet_1_1GhostedAreaSpec.html#a8c9d4cac9f2fe977a2a750f045387432

According to that manual, set_fj2_placement is deprecated in FastJet 3 and simply avoiding setting it to true let the code stick on the FJ3 ghost placement.

All this seems good!

I have one question for you: based on what the comparisons "look as expected"? The changes in shape
look minor, but from what can you deduce that there was an improvement in them? Do you have any study for it that you can point out, or do you consider it better simply because it follow now the recommandations of the authors of that code? (Which is not wrong, in principle, by the way!)

@rappoccio
Copy link
Contributor Author

@perrotta Right. The changes were minor. We shouldn't see very much improvement (very slight improvement in resolution of area). We are now just following their recommendations.

@perrotta
Copy link
Contributor

perrotta commented Mar 1, 2018

Modifications in pat::jet are visible in the miniAOD outputs of the jenkins tests, with effect propagated to the correlated quantities. Curiously enough, the only workflows run in the automatic tests in which there is some effect also on reco::jet's are the HeavyIons ones:

  • 150.0_HydjetQ_B12_5020GeV_2018, e.g.
    image

  • 140.53_RunHI2011, e.g.
    image

For the standard workflows, I ran some slightly larger stats (400 events) with two potenzially affected workflows, i.e. TTbar with PU and highPt QCD. The differences wrt baseline are in line with what observed with lower statistics in the automaticjenkins tests:

  • QCD13TeVPt3Ts3T5in2018 wf 10859:

all_mini_testpr22305vsorig_qcd13tevpt3ts3t5in2018wf10859p0c_log10patjets_slimmedjets__reco_obj_energy
image
untitled

  • TTbar13TeVPU2017 wf 10224:

image
image

@perrotta
Copy link
Contributor

perrotta commented Mar 1, 2018

+1

  • This PR addresses the warnings that appeared after the move to the new 3.3.0 fastjet version (see also issue Compile warnings in CMSSW after integration of new fastjet  #22297). To do so it complies to the recommendation of the authors of fastjet of replacing deprecated methods and ghost placement with the updated and preferred ones
  • The move from FJ2 to FJ3 ghost placement has some expected effect on the jet outputs and all related variables. There are however no visible drawbacks in the move, which goes in the direction of getting fully compliant with the last version of fastjet (already implemented in CMSSW)

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 1, 2018

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

@rappoccio
Copy link
Contributor Author

Indeed the jet corrections are only ever applied at the MINIAOD level for pp collisions.

@fabiocos
Copy link
Contributor

fabiocos commented Mar 1, 2018

+1

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

5 participants