Adding new GCP trends tool and commenting unnecessary print out in Trend #40336

perezdanyer · 2022-12-15T19:12:18Z

PR description:

The aim of this PR is to add a new tool to be used by the Tracker Alignment (TrkAl) team when deriving performance trends over data-taking periods. The tool adds the capability of performing now trends of position estimators when doing tracker geometry comparisons, which integrates to the already existing trend tools for the other internal validation procedures performed in TrkAl.

The changes consist in the following:

addition of main code corresponding to GCPtrend tool
minor modification in Trend library to avoid large print out

PR validation:

Tests of the tool have been performed by both the author and another member of the TrkAl team.

@consuegs , @connorpa , @henriettepetersen, @antoniovagnerini, @mmusich

cmsbuild · 2022-12-15T19:19:25Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40336/33423

This PR adds an extra 16KB to repository

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

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40336/33423/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40336/33423/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2022-12-15T21:22:40Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40336/33424

This PR adds an extra 20KB to repository

cmsbuild · 2022-12-15T21:23:06Z

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

It involves the following packages:

Alignment/OfflineValidation (alca)

@malbouis, @yuanchao, @cmsbuild, @saumyaphor4252, @francescobrivio, @ChrisMisan, @tvami can you please review it and eventually sign? Thanks.
@mmusich, @adewit, @tocheng, @tlampen 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

tvami · 2022-12-15T21:53:45Z

@cmsbuild , please test

cmsbuild · 2022-12-16T01:51:05Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9180bd/29645/summary.html
COMMIT: abe8f55
CMSSW: CMSSW_13_0_X_2022-12-15-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/40336/29645/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3557521
DQMHistoTests: Total failures: 9
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3557490
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 211 log files, 162 edm output root files, 49 DQM output files
TriggerResults: no differences found

mmusich · 2022-12-16T10:48:21Z

Alignment/OfflineValidation/bin/GCPtrends.cc

+#include <boost/property_tree/ptree.hpp>
+#include <boost/property_tree/json_parser.hpp>
+
+#include "TROOT.h"


is this #include needed?

mmusich · 2022-12-16T10:50:09Z

Alignment/OfflineValidation/bin/GCPtrends.cc

+  assert(lumifile.good());
+  const Run2Lumi lumi_per_run(lumibyrunfile, firstrun, lastrun, 1000);
+
+  std::map<TString, Int_t> comparison_map;


I think C++ native types are generally preferred to ROOT types, but perhaps it's not mandatory here, as this is essentially a plotting macro.

mmusich · 2022-12-16T10:53:43Z

Alignment/OfflineValidation/bin/GCPtrends.cc

+
+  // wheels/layers per tracker phase: <pahse,<sublevel,number of layers/wheels>>
+  const std::map<Int_t, std::map<Int_t, Int_t>> Phase_Subdetector_Layers_Map = {
+      {0, {{1, 3}, {2, 2}}}, {1, {{1, 4}, {2, 3}}}, {2, {{1, 4}, {2, 10}}}};


there are twelve Disks in the phase-2 tracker and not 10.

also the access pattern of std::map of std::maps is not really user-friendly... perhaps this could be recast into a class with accessors

Thanks a lot @mmusich for the comments, and sorry for the delay (I was offline). I have implemented most of your comments, and my only minor follow up is on this one; I took the numbers from this table on the relevant package on CMSSW, maybe that is outdated then? In any case I trust your plot :)

I took the numbers from this table on the relevant package on CMSSW, maybe that is outdated then?

Indeed, it looks like it was not updated.

Anyway you don't have to trust me, construct a TrackerGeometry object and see it for yourself ;)
For reference, here's the output of this test in the same package you pointed out:

cmssw/Geometry/TrackerGeometryBuilder/test/BuildFile.xml

Lines 37 to 41 in c628e55

<library file="PixelTopologyMapTest.cc" name="PixelTopologyMapTest">

<use name="DataFormats/TrackerCommon"/>

<use name="Geometry/Records"/>

<flags EDM_PLUGIN="1"/>

</library>

https://cmssdt.cern.ch/SDT/cgi-bin/logreader/slc7_amd64_gcc11/CMSSW_13_0_X_2023-01-16-1100/unitTestLogs/Geometry/TrackerGeometryBuilder#/82020

mmusich · 2022-12-16T10:55:57Z

Alignment/OfflineValidation/bin/GCPtrends.cc

+
+      Int_t sublevel_idx = *sublevel.Get();
+
+      Histos[sublevel_idx][0]->Fill(*dx.Get() * 10000);


could the 10000 repeated here and elsewhere be a named constants (I guess it's a cm -> µm conversion factor)

mmusich · 2022-12-16T10:57:15Z

Alignment/OfflineValidation/bin/GCPtrends.cc

+        Double_t sigma = Histos[level.first][variable.first]->GetStdDev();
+
+        Graphs[level.first][variable.first][0]->AddPoint(run_number, mean);
+        if (fabs(mean) > Graphs[level.first][variable.first][0]->GetMaximum() && fabs(mean) > 0.) {


std::abs is preferred to fabs

mmusich · 2022-12-16T10:59:09Z

Alignment/OfflineValidation/src/Trend.cc

@@ -22,10 +22,10 @@ namespace pt = boost::property_tree;

 Run2Lumi::Run2Lumi(fs::path file, int first, int last, float convertUnit)
    : firstRun(first), lastRun(last), convertUnit(convertUnit) {
-  cout << __func__ << endl;
+  //cout << __func__ << endl;


leaving a lot of commented code is not a good option. Either remove or add a verbose flag in order to activate the couts (by the way it's technically not thread-safe and in general MessageLogger is adivsed).

ChrisMisan · 2022-12-29T13:15:50Z

@perezdanyer what is the status of this PR?

cmsbuild · 2023-01-16T12:27:39Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40336/33751

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)

code-format:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40336/33751/code-format.patch
e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40336/33751/code-format.patch | patch -p1
You can also run scram build code-format to apply code format directly

cmsbuild · 2023-01-17T19:12:14Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40336/33783

This PR adds an extra 16KB to repository

cmsbuild · 2023-01-17T19:12:40Z

Pull request #40336 was updated. @malbouis, @yuanchao, @cmsbuild, @saumyaphor4252, @francescobrivio, @ChrisMisan, @tvami can you please check and sign again.

mmusich · 2023-01-17T19:13:39Z

please test

cmsbuild · 2023-01-17T21:46:18Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9180bd/30046/summary.html
COMMIT: f6098af
CMSSW: CMSSW_13_0_X_2023-01-17-1100/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/40336/30046/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

There are some workflows for which there are errors in the baseline:
11634.15 step 3
The results for the comparisons for these workflows could be incomplete
This means most likely that the IB is having errors in the relvals.The error does NOT come from this pull request

Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 49
DQMHistoTests: Total histograms compared: 3555538
DQMHistoTests: Total failures: 0
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3555516
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 48 files compared)
Checked 211 log files, 162 edm output root files, 49 DQM output files
TriggerResults: no differences found

tvami · 2023-01-18T00:57:55Z

+alca

cmsbuild · 2023-01-18T00:58:22Z

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

perrotta · 2023-01-18T07:00:35Z

+1

cmsbuild added this to the CMSSW_13_0_X milestone Dec 15, 2022

cmsbuild added alca-pending code-checks-pending orp-pending pending-signatures tests-pending labels Dec 15, 2022

cmsbuild added code-checks-rejected code-checks-pending and removed code-checks-pending code-checks-rejected labels Dec 15, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 15, 2022

cmsbuild added tests-started and removed tests-pending labels Dec 15, 2022

cmsbuild added tests-approved and removed tests-started labels Dec 16, 2022

mmusich suggested changes Dec 16, 2022

View reviewed changes

cmsbuild mentioned this pull request Dec 16, 2022

Add DiMuonValidation to the TkAl "new" all-in-one tool #40346

Merged

cmsbuild added code-checks-pending tests-pending and removed tests-approved code-checks-approved labels Jan 16, 2023

cmsbuild removed the code-checks-pending label Jan 16, 2023

cmsbuild added alca-pending code-checks-pending pending-signatures tests-pending and removed alca-approved fully-signed tests-approved code-checks-approved labels Jan 17, 2023

cmsbuild added code-checks-approved and removed code-checks-pending labels Jan 17, 2023

cmsbuild added tests-started and removed tests-pending labels Jan 17, 2023

cmsbuild added tests-approved and removed tests-started labels Jan 17, 2023

cmsbuild added alca-approved fully-signed and removed alca-pending pending-signatures labels Jan 18, 2023

cmsbuild added orp-approved and removed orp-pending labels Jan 18, 2023

cmsbuild merged commit 01b463b into cms-sw:master Jan 18, 2023

This was referenced Jan 18, 2023

[Tensorflow] Build with GPU enabled cms-sw/cmsdist#7648

Merged

[SCRAM] Properly restore and set the build time env cms-sw/cmsdist#8254

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding new GCP trends tool and commenting unnecessary print out in Trend #40336

Adding new GCP trends tool and commenting unnecessary print out in Trend #40336

perezdanyer commented Dec 15, 2022

cmsbuild commented Dec 15, 2022

cmsbuild commented Dec 15, 2022

cmsbuild commented Dec 15, 2022

tvami commented Dec 15, 2022

cmsbuild commented Dec 16, 2022

mmusich Dec 16, 2022

mmusich Dec 16, 2022

mmusich Dec 16, 2022 •

edited

Loading

perezdanyer Jan 16, 2023

mmusich Jan 16, 2023

mmusich Jan 16, 2023

mmusich Dec 16, 2022

mmusich Dec 16, 2022

mmusich Dec 16, 2022

ChrisMisan commented Dec 29, 2022

cmsbuild commented Jan 16, 2023

cmsbuild commented Jan 17, 2023

cmsbuild commented Jan 17, 2023

mmusich commented Jan 17, 2023

cmsbuild commented Jan 17, 2023

tvami commented Jan 18, 2023

cmsbuild commented Jan 18, 2023

perrotta commented Jan 18, 2023

	<library file="PixelTopologyMapTest.cc" name="PixelTopologyMapTest">
	<use name="DataFormats/TrackerCommon"/>
	<use name="Geometry/Records"/>
	<flags EDM_PLUGIN="1"/>
	</library>


		Int_t sublevel_idx = *sublevel.Get();

		Histos[sublevel_idx][0]->Fill(dx.Get() 10000);

Adding new GCP trends tool and commenting unnecessary print out in Trend #40336

Adding new GCP trends tool and commenting unnecessary print out in Trend #40336

Conversation

perezdanyer commented Dec 15, 2022

PR description:

PR validation:

cmsbuild commented Dec 15, 2022

cmsbuild commented Dec 15, 2022

cmsbuild commented Dec 15, 2022

tvami commented Dec 15, 2022

cmsbuild commented Dec 16, 2022

Comparison Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmusich Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisMisan commented Dec 29, 2022

cmsbuild commented Jan 16, 2023

cmsbuild commented Jan 17, 2023

cmsbuild commented Jan 17, 2023

mmusich commented Jan 17, 2023

cmsbuild commented Jan 17, 2023

Comparison Summary

tvami commented Jan 18, 2023

cmsbuild commented Jan 18, 2023

perrotta commented Jan 18, 2023

mmusich Dec 16, 2022 •

edited

Loading