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

Inclusion of a new tool for easier creation of comparison plots and several minor fixes #21457

Merged
merged 17 commits into from
Dec 26, 2017

Conversation

mteroerd
Copy link
Contributor

This PR contains changes of five different categories which are divided into five different commits:

  1. In my previous PR, I added a functionality to automate the waiting times between different iterations. This is now also added to the MC workflow. (c06ca14)

  2. In macros/DrawIteration.C and macros/DrawPlot.C, there were several instances of 0 being used instead of nullptr. This is fixed now. Previously, using DrawPlot.C caused a Segmentation Fault when exiting the program. This is also fixed. (fc79c15)

  3. The used EDAnalyzers in plugins/ are now multi-threading-aware. (e287898)

  4. Instead of using a custom TTRHBuilder, now the WithAngleAndTemplate TTRHBuilder is used by default. (866f818)

  5. This is the biggest change. A tool was added to create trees which contain custom APE values. These trees can be used for comparison plots in the APE tool. Previously, this was only possible using a workaround. The EDAnalyzer code contains code snippets from both plugins/ApeEstimator.cc and plugins/ApeEstimatorSummary.cc. (ff06640)

@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/PR-21457/2184

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

Alignment/APEEstimation

@ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @lpernie can you please review it and eventually sign? Thanks.
@mschrode, @ghellwig, @mmusich, @tocheng this is something you requested to watch as well.
@davidlange6, @slava77 you are the release manager for this.

cms-bot commands are listed here

@ghellwig
Copy link

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 24, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/24676/console Started: 2017/11/24 17:30



const LocalError& errHitWoApe = errorWithoutAPE;
const LocalError& errTrk = tsos.localError().positionError();

const StatePositionAndError2 positionAndError2Hit = this->positionAndError2(lPHit, errHitApe, hit);
const StatePositionAndError2 positionAndError2HitWoApe = this->positionAndError2(lPHit, errHitWoApe, hit);
std::cout<<"errHitWoApe " <<errHitWoApe<<"errHitApe "<<errHitApe<<std::endl;
std::cout<<"errHitWoApe " <<errHitWoApe<<"errHitApe "<<errHitApe<<std::endl;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mteroerd as you are touching this: you have to use the MessageLogger instead of std::cout

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
else{
apeX2 = std::pow(smoothFraction*std::sqrt(apeX2new) + (1-smoothFraction)*std::sqrt(apeX2old), 2);
apeY2 = std::pow(smoothFraction*std::sqrt(apeY2new) + (1-smoothFraction)*std::sqrt(apeY2old), 2);
apeY2 = std::pow(smoothFraction*std::sqrt(apeY2new) + (1-smoothFraction)*std::sqrt(apeY2old), 2);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mteroerd the performance of std::pow(A, 2) is worse than A*A, i.e. I suggest to compute A and then use A*A instead of std::pow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

DefaultApeTree::sectorBuilder()
{
// Same procedure as in ApeEstimator.cc
TFile* tkTreeFile(TFile::Open((parameterSet_.getParameter<std::string>("TrackerTreeFile")).c_str()));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mteroerd I believe it is recommended to use the TFileService, but @Dr15Jones might correct me.
At least, if you would use it you would have to specify this in the declaration of the Analyzer class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in addition, use FileInPath here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find any documentations on TFileService or FileInPath that really explain how to correctly implement them or why they are better in this context. Can you link me to that, please?
Neither https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideEdmFileInPath nor https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideTFileService are helpful to me.


process.DefaultApeTree = cms.EDAnalyzer('DefaultApeTree',
resultFile = cms.string(theOutputFile),
TrackerTreeFile = cms.string(os.environ['CMSSW_BASE'] + '/src/Alignment/TrackerAlignment/hists/TrackerTree.root'),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this only works if you have checked out Alignment/TrackerAlignment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true. Actually, you also need to run the TrackerTreeGenerator. But this is the standard procedure when setting up the APE tool. It is also written in the guide for setting up the tool:
https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideAlignmentPositionErrorEstimator#Setting_up_the_Tool

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let me propose the following for the next update:
you can conditionally run the tracker tree generator directly in this config, if the TrackerTree file does not exist (best not in a location within Alignment/TrackerAlignment to avoid the requirement of checking it out).

@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/PR-21457/2189

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #21457 was updated. @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @lpernie can you please check and sign again.

@ghellwig
Copy link

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Dec 18, 2017

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/25153/console Started: 2017/12/18 13:58

@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-21457/25153/summary.html

@slava77 comparisons for the following workflows were not done due to missing matrix map:

  • /build/cmsbld/jenkins-workarea/workspace/compare-root-files-short-matrix/results/JR-comparison/PR-21457/1325.7_TTbar_13_94XNanoAODINPUT+TTbar_13_94XNanoAODINPUT+NANOEDMMC2017

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 17 differences found in the comparisons
  • DQMHistoTests: Total files compared: 27
  • DQMHistoTests: Total histograms compared: 2836825
  • DQMHistoTests: Total failures: 2542
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2834105
  • DQMHistoTests: Total skipped: 178
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 1.14000000001 KiB( 23 files compared)
  • Checked 113 log files, 9 edm output root files, 27 DQM output files

@ghellwig
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 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)

@davidlange6
Copy link
Contributor

merge

@cmsbuild cmsbuild merged commit 4cce36e into cms-sw:master Dec 26, 2017
@mteroerd mteroerd deleted the ape-new-features branch November 20, 2018 10:39
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