Introduction of New-All-In-One tool for Offline Validation of TrkAlignment #38304

antoniovagnerini · 2022-06-09T13:32:45Z

PR description:

The aim of this PR is to introduce a new "All-In-One-Tool" common interface for the offline validation of the alignment. In particular, the validations affected are the functionality of DMR, PV (and relative trends), Geometry Comparisons as well as the JetHT validation.

The main improvements consist in the following:

semi-automatic analysis using CRAB
compatibility with Python3
git for output tracking
common plotting style for trends

PR validation:

Unit test have been devised for each validation type to ensure correctness of the outcome.

@connorpa @consuegs @mmusich

cmsbuild · 2022-06-09T13:37:15Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38304/30467

ERROR: Unable to merge PR.

See log https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38304/30467/cms-checkout-topic.log

tvami · 2022-06-09T15:43:02Z

@antoniovagnerini seems we have a merge conflict

CONFLICT (content): Merge conflict in Alignment/OfflineValidation/scripts/validateAlignments.py
Auto-merging Alignment/OfflineValidation/test/BuildFile.xml
CONFLICT (content): Merge conflict in Alignment/OfflineValidation/test/BuildFile.xml

Please resolve conflict and commit again.

Also having more commits than the number of files changed... that's somewhat discouraged. Please group them together into a few bigger commits with the squash option of git.

tvami · 2022-06-09T15:44:01Z

Oh also, just looking at the first file, .gitignore should not be commited.

mmusich · 2022-06-09T16:45:47Z

Also what does "aligned" in the title mean?

tvami · 2022-06-09T18:39:49Z

type trk

tvami · 2022-06-10T13:06:33Z

@cms-sw/orp-l2 can you remind me what's our review policy for files under /bin/? Is that the same policy as for /test/?

tvami · 2022-06-10T13:07:09Z

@antoniovagnerini et al, I see a few files added under /data/, so those should technically go into the cms-data repository not the main repo here

tvami · 2022-06-10T13:14:25Z

Alignment/OfflineValidation/interface/PreparePVTrends.h

+#define logInfo std::cout << "INFO: "
+#define logWarning std::cout << "WARNING: "
+#define logError std::cout << "ERROR!!! "


Wouldnt it have been easier to just used the MsgLogger variants of this?

in the original implementation this was part of a ROOT macro which was supposed to work also without a cms environment. Given this is now in interface (even though it not very clear to me why it should be the case....) and so by construction implied to be distributed with cmssw, it probably makes sense to change it to MsgLogger.

tvami · 2022-06-10T13:16:38Z

Alignment/OfflineValidation/interface/SmartSelectionMonitor.h

+public:
+
+  SmartSelectionMonitor(){}
+  ~SmartSelectionMonitor() { }


Suggested change

~SmartSelectionMonitor() { }

~SmartSelectionMonitor() = default;

Addressed in next commit.

tvami

Some cleanup/cosmetic changes / memory leak suggestion for new

tvami · 2022-06-10T13:17:19Z

Alignment/OfflineValidation/interface/SmartSelectionMonitor.h

+    h->Reset("ICE");
+    h->SetDirectory(gROOT);
+    (*map)[tag] = h; 
+    //    printf("new histo created with name = %30s and tag = %15s: Name=%s\n",allName.c_str(), tag.c_str(), h->GetName());


Remove commented out code

Addressed in next commit.

tvami · 2022-06-10T13:17:27Z

Alignment/OfflineValidation/interface/SmartSelectionMonitor.h

+	if(h->second->GetEntries()>0)neverFilled = false;
+
+	if(h->first=="all"){h->second->SetName(Form("%s_%s",h->first.c_str(),h->second->GetName()));}
+	//printf("histo = %30s tag = %15s Name = %s\n",it->first.c_str(), h->first.c_str(),  h->second->GetName());


Addressed in next commit.

tvami · 2022-06-10T13:18:13Z

Alignment/OfflineValidation/interface/Trend.h

+#include <TROOT.h>
+#include <TCanvas.h>
+#include <TLegend.h>
+//#include <TH1.h> // TODO


is this still a TODO?

Removed in next commit.

tvami · 2022-06-10T13:19:14Z

Alignment/OfflineValidation/macros/makeArrowPlots.C

+  //float dZTest =2;
+  //float xLegObj = 20;
+  //float yLegObj = 18;



Cleaned in next commit.

tvami · 2022-06-10T13:19:49Z

Alignment/OfflineValidation/macros/makeArrowPlots.C

  //Plot10Mu( name, xLim/2, yLim, 0.2*xLim );
  TPaveText* atext = new TPaveText(0.2*xLim,0.85*yLim,0.66*xLim,0.99*yLim);
+  //atext->AddText(sliceLeg);


you added a commented line... either explain why this might be needed or remove

tvami · 2022-06-10T13:20:09Z

Alignment/OfflineValidation/macros/makeArrowPlots.C

+  //char outfile[192];
+  //sprintf( outfile, "%s/%s.png", outputDir_, name );
+  //OBPCanvas->Print( outfile );
+  std::ostringstream outfile_s;
+  outfile_s << outputDir_ << "/" << name << ".png";
+  TString outfile(outfile_s.str());
  OBPCanvas->Print( outfile );
-  sprintf(outfile, "%s/%s.pdf", outputDir_, name);
-  OBPCanvas->SaveAs(outfile);
+  std::ostringstream outfile2_s;
+  outfile2_s << outputDir_ << "/" << name << ".pdf";
+  TString outfile2(outfile2_s.str());
+  //sprintf(outfile, "%s/%s.pdf", outputDir_, name);
+  //OBPCanvas->SaveAs(outfile);


Yep. Generally I left only quite an extensive snippet of code with makeRZArrowPlot function commented out. I believe that is unnecessary to remove.

tvami · 2022-06-10T13:22:07Z

Alignment/OfflineValidation/plugins/JetHTAnalyzer.cc

+  //Specify that only 'tracks' is allowed
+  //To use, remove the default given above and uncomment below
+  //ParameterSetDescription desc;
+  //desc.addUntracked<edm::InputTag>("tracks","ctfWithMaterialTracks");
+  //descriptions.addDefault(desc);


this default text could be cleaned up

That explains why text bellow is commented out and when it should be un-commented. Therefore I would leave it.

actually a real fillDescriptions method should be provided and the boiler plate code removed.

fillDescriptions was added

tvami · 2022-06-10T13:23:09Z

Alignment/OfflineValidation/plugins/PixelBaryCentreAnalyzer.cc

+  vFPIX_plus_ = SimplePoint(FPIX_plus_);
+  vFPIX_minus_ = SimplePoint(FPIX_minus_);
+
+  for (unsigned int i = 0; i < 4; i++) {


Magic numbers like the 4 here and the 3 below should be made a named constant (especially since this is going to change for Phase-2)

Changed within next commit.

tvami · 2022-06-10T13:37:43Z

Alignment/OfflineValidation/src/PrepareDMRTrends.cc

+    }
+  }
+  fout->Close();
+}


the things that are new-ed technically should be delete-ed as well in the end

Pointers properly dealt with.

tvami · 2022-06-10T13:38:40Z

Alignment/OfflineValidation/test/BuildFile.xml

+  <!-- <bin file="testAlignmentOfflineValidation.cpp" name="PVsingle"> -->
+  <!--   <flags TEST_RUNNER_ARGS=" /bin/bash Alignment/OfflineValidation/test test_all.sh"/> -->
+  <!--   <use name="FWCore/Utilities"/> -->
+  <!-- </bin> -->
+  <!-- <bin file="testPVPlotting.cpp"> -->
+  <!--   <flags PRE_TEST="PVsingle"/> -->
+  <!--   <use name="rootmath"/> -->
+  <!--   <use name="roothistmatrix"/> -->
+  <!--   <use name="rootgraphics"/> -->
+  <!--   <use name="Alignment/OfflineValidation"/> -->
+  <!-- </bin> -->


Are these not needed in the end?

these should definitely be maintained.

This was fully changed.

mmusich · 2022-06-13T07:29:24Z

assign trk-dpg

TomasKello · 2022-06-13T08:47:42Z

I am having a look at suggested changes and will try to perform rebase. It will take me some time, so please be patient. It is also likely I will add some small changes with fully new commits. In any case you can contact me here.

mmusich

here's some preliminary set of comments.
I might post more, once the preliminary set is addressed.

mmusich · 2022-06-13T09:07:03Z

Alignment/OfflineValidation/bin/toolbox.h

+
+namespace AllInOneConfig {
+
+  //boost::property_tree::ptree get_unique_child (const boost::property_tree::ptree& tree)


is all of this commented text needed?

Actually this namespace is barely used as far as I can tell only in Alignment/OfflineValidation/bin/DMRsingle.cc and I see no previous reference to get_unique_child that could hint its usage. ---> Removing this commented part with the next commit.

mmusich · 2022-06-13T09:08:24Z

.gitignore

+*.ini
+*.old
+*.bckp
+*.info


I am not sure these changes are appropriate.

Ok we do no keep .gitignore anymore.

mmusich · 2022-06-13T09:11:35Z

Alignment/OfflineValidation/BuildFile.xml

@@ -25,6 +25,8 @@
 <use name="CLHEP"/>
 <use name="rootmath"/>
 <use name="roothistmatrix"/>
+<use name="rootgraphics"/>
+<lib name="MultiProc" />


this is also in the Alignment/OfflineValidation/bin/BuildFile.xml.
Is it needed in both places?

Seems not to be needed in /bin. Removing.

mmusich · 2022-06-13T09:12:03Z

Alignment/OfflineValidation/README.md

+validateAlignments.py $CMSSW_BASE/src/Alignment/OfflineValidation/test/test.yaml 
+
+-----------------------------------------------------------------------
+File for submitting this DAG to HTCondor           : /afs/cern.ch/user/d/dbrunner/ToolDev/CMSSW_10_6_0/src/MyTest/DAG/dagFile.condor.sub


please don't use personal user accounts in README files.

mmusich · 2022-06-13T09:12:29Z

Alignment/OfflineValidation/README.md

+ - from DMR toy to real application
+   - GCP (get "n-tuples" + grid, 3D, TkMaps)
+   - DMRs (single + merge + trend)
+   - PV (single + merge + trend)


aren't some of these already implemented??

Yep, updated.

mmusich · 2022-06-13T11:06:00Z

Alignment/OfflineValidation/python/TkAlAllInOneTool/GCP_Ntuples_cfg.py

+
+##Read in AllInOne config in JSON format
+with open(options.config, "r") as configFile:
+    config = _byteify(json.load(configFile, object_hook=_byteify),ignore_dicts=True)


I thought the _byteify was not needed anymore?

That was removed to make runtests work.

mmusich · 2022-06-13T11:06:22Z

Alignment/OfflineValidation/python/TkAlAllInOneTool/GCP_TkAlMap.py

+    TkMap_drange_full   = TkAlMap('test', title, in_file, use_default_range=True,  two_sigma_cap=False, GEO_file=geometry_file, tracker='full',   palette=palette, check_tracker=auto_tk)
+    TkMap_drange_pixel  = TkAlMap('test', title, in_file, use_default_range=True,  two_sigma_cap=False, GEO_file=geometry_file, tracker='pixel',  palette=palette, check_tracker=auto_tk)
+    TkMap_drange_strips = TkAlMap('test', title, in_file, use_default_range=True,  two_sigma_cap=False, GEO_file=geometry_file, tracker='strips', palette=palette, check_tracker=auto_tk)
+


please remove empty lines at the end of this file.

mmusich · 2022-06-13T11:12:34Z

Alignment/OfflineValidation/src/PlotAlignmentValidation.cc

@@ -541,12 +499,16 @@ void PlotAlignmentValidation::plotSS(const std::string& options, const std::stri
  bool plotLayers = false;  // overrides plotLayerN
  //  bool plotRings  = false;  // Todo: implement this?
  bool plotSplits = false;
+  if (plotSplits)
+    ;


what's the point of this change?

No idea. Removed.

Now I remembered. This actually is needed for a reason I have never understood. Compiler seems to throw "[-Werror=unused-but-set-variable" error eventhough 'plotSplits' is assigned in if-statement right after.

is plotSplits used anywhere else below?

yes multiple times in assignments within or out of if-statements and few times in the condition itself

ah it is late ... indeed in that member function it is unused. Will remove it entirely in the second commit.

mmusich · 2022-06-13T11:15:13Z

Alignment/OfflineValidation/test/GCP_defaultRange.json

@@ -0,0 +1,19 @@
+{


does this file belong here?

Indeed, this can be moved away. Please approve this PR: cms-data/Alignment-OfflineValidation#3

this PR can be tested with the cms-data one, but I would suggest to push also the txt files in Alignment/OfflineValidation/data in the same PR

Yes it is tested, GCP unit test still works plus I moved file to local data dir.

mmusich · 2022-06-13T11:15:30Z

Alignment/OfflineValidation/test/BuildFile.xml

+  <!--   <flags TEST_RUNNER_ARGS=" /bin/bash Alignment/OfflineValidation/test test_all.sh"/> -->
+  <!--   <use name="FWCore/Utilities"/> -->
+  <!-- </bin> -->
+  <!-- <bin file="testPVPlotting.cpp"> -->


please don't remove this test.

This one had 'test_all.sh' as a pre-requisite, however that required the old version of validateAlignments.py to be run. Plotting libraries in testPVPlotting.cpp are anyway implicitly used in unit test "PVall" so I could see no point in preserving this test.

tvami · 2022-06-15T09:28:29Z

Hi @antoniovagnerini given that there is a lot to do with this PR, would you mind changing it's status to a "draft PR"? Thanks!

tvami · 2022-11-04T17:26:00Z

oh wow #39967 is though PR... affecting MC in all times with widespread changes...

tvami · 2022-11-04T17:26:49Z

+alca

changes are coming from a different PR
this PR's code changes are in line with the PR desc and the follow-up to the review, thanks @TomasKello

mmusich · 2022-11-08T12:48:11Z

I think this is still pending either @sroychow or @connorpa signatures

sroychow · 2022-11-08T13:54:45Z

+1

cmsbuild · 2022-11-08T13:55:35Z

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)

mmusich · 2022-11-08T13:58:04Z

A note to @cms-sw/orp-l2: when merging, this PR requires cms-data/Alignment-OfflineValidation#3 to be merged at the same time.

rappoccio · 2022-11-08T20:16:52Z

+1

Extensive review but seems to have converged.

mmusich · 2022-11-09T06:54:56Z

This severely broke the IB: https://cmssdt.cern.ch/SDT/cgi-bin/showBuildLogs.py/el8_amd64_gcc11/www/tue/12.6-tue-23/CMSSW_12_6_X_2022-11-08-2300

…ompilationEL8 fix `gcc11` compilation issues in `CompareAlignments`, post #38304 merge

cmsbuild added this to the CMSSW_12_5_X milestone Jun 9, 2022

cmsbuild added alca-pending code-checks-pending core-pending orp-pending pending-signatures tests-pending labels Jun 9, 2022

cmsbuild added code-checks-rejected and removed code-checks-pending labels Jun 9, 2022

cmsbuild added the trk label Jun 9, 2022

tvami reviewed Jun 10, 2022

View reviewed changes

cmsbuild added the trk-dpg-pending label Jun 13, 2022

antoniovagnerini changed the title ~~New all in one tool aligned~~ Introduction of New-All-In-One tool for Offline Validation of TrkAlignment Jun 13, 2022

mmusich reviewed Jun 13, 2022

View reviewed changes

antoniovagnerini marked this pull request as draft June 16, 2022 11:48

TomasKello force-pushed the newAllInOneToolAligned branch from e0e86be to 24a89c0 Compare July 8, 2022 14:38

cmsbuild added code-checks-pending and removed code-checks-rejected labels Jul 8, 2022

cmsbuild added alca-approved and removed alca-pending labels Nov 4, 2022

cmsbuild added fully-signed trk-dpg-approved and removed pending-signatures trk-dpg-pending labels Nov 8, 2022

cmsbuild added orp-approved and removed orp-pending labels Nov 8, 2022

cmsbuild merged commit ec95b0d into cms-sw:master Nov 8, 2022

smuzaffar mentioned this pull request Nov 8, 2022

Update tag for Alignment-OfflineValidation to V00-03-00 cms-sw/cmsdist#8174

Merged

mmusich added a commit to mmusich/cmssw that referenced this pull request Nov 9, 2022

fix compilation issues in CompareAlignments, post cms-sw#38304 merge

2030dbb

mmusich mentioned this pull request Nov 9, 2022

fix gcc11 compilation issues in CompareAlignments, post #38304 merge #40019

Merged

cmsbuild mentioned this pull request Nov 9, 2022

Add G4HepEm to CMSSW #39736

Merged

mmusich added a commit to mmusich/cmssw that referenced this pull request Nov 9, 2022

fix compilation issues in CompareAlignments, post cms-sw#38304 merge

4136730

cmsbuild added a commit that referenced this pull request Nov 10, 2022

Merge pull request #40019 from mmusich/fixAlignmentOfflineValidationC…

193adba

…ompilationEL8 fix `gcc11` compilation issues in `CompareAlignments`, post #38304 merge

cramonal pushed a commit to cramonal/cmssw that referenced this pull request Nov 14, 2022

fix compilation issues in CompareAlignments, post cms-sw#38304 merge

4a4b829

aandvalenzuela mentioned this pull request Nov 16, 2022

[ROOT6] Segfault in test DMRall on Alignment/OfflineValidation #40091

Closed

mmusich mentioned this pull request Dec 2, 2022

minor improvements to Alignment/OfflineValidation #40222

Merged

mmusich mentioned this pull request Dec 16, 2022

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

Merged

mmusich mentioned this pull request Nov 24, 2023

[TkAl] remove "old" all-in-one metatool from cmssw #43386

Merged

mmusich mentioned this pull request Jan 3, 2024

Fix TkAlStyle includes and add unit tests #43650

Merged

mmusich mentioned this pull request Jan 12, 2024

Implementation of the Muon Track Splitting (MTS) Validation in the new TkAl all-in-one tool #43708

Merged

	~SmartSelectionMonitor() { }
	~SmartSelectionMonitor() = default;


		namespace AllInOneConfig {

		//boost::property_tree::ptree get_unique_child (const boost::property_tree::ptree& tree)

Introduction of New-All-In-One tool for Offline Validation of TrkAlignment #38304

Introduction of New-All-In-One tool for Offline Validation of TrkAlignment #38304

Conversation

antoniovagnerini commented Jun 9, 2022

PR description:

PR validation:

cmsbuild commented Jun 9, 2022

tvami commented Jun 9, 2022

tvami commented Jun 9, 2022

mmusich commented Jun 9, 2022

tvami commented Jun 9, 2022

tvami commented Jun 10, 2022 • edited

tvami commented Jun 10, 2022

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

tvami left a 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

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

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

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

mmusich commented Jun 13, 2022

TomasKello commented Jun 13, 2022

mmusich left a 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

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

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

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

Choose a reason for hiding this comment

tvami commented Jun 15, 2022

tvami commented Nov 4, 2022

tvami commented Nov 4, 2022

mmusich commented Nov 8, 2022

sroychow commented Nov 8, 2022

cmsbuild commented Nov 8, 2022

mmusich commented Nov 8, 2022

tvami commented Jun 10, 2022 •

edited