[WIP] Z->mm in to new all in one tool #21

mmusich · 2021-05-12T10:02:22Z

Just starting to push the existing code from https://github.com/gu180/TrackderAlignment_DiMuonValidation/tree/all-in-one.
@connorpa @vbotta @antoniovagnerini

connorpa · 2021-05-26T09:40:49Z

It seems that certain options in testZmumu.yaml are not used in Zmumu_cfg.py, e.g. maxevents or usefit. Make sure that the given options are used or remove them from the example.

In Zmumumerge.cc, in the Zmumumerge function:

I am not sure to understand why you clear newly created vectors.
A ptree validation is declared but not used: don't you need it to extract certain information from the yaml file? Like the minimum or the legendoptions? We should be consistent.
I am not sure to understand why you extract information from the ptree alignments to fill several vectors, and then use the vectors instead of the ptree in a later loop. You could shorten your code by using directly the information from the ptree in the loop where you call the Fitting... and Draw... functions.

mmusich · 2021-05-26T09:44:56Z

Just to be clear, that's not my code.... it's An Gu (@gu180)

connorpa · 2021-06-10T07:04:02Z

@gu180 Are you working on it?

mmusich · 2021-06-10T17:21:16Z

@connorpa please introduce comments directly within the code. I think that would substantially speed up the review.

gu180 · 2021-06-10T17:22:31Z

I am still trying to include ome other variables in the configuration. Best Regards. An Gu

…

________________________________ 发件人: Patrick Connor ***@***.***> 发送时间: 2021年6月10日 3:04 收件人: cms-trackeralign/cmssw ***@***.***> 抄送: An Gu ***@***.***>; Mention ***@***.***> 主题: Re: [cms-trackeralign/cmssw] [WIP] Z->mm in to new all in one tool (#21) @gu180<https://github.com/gu180> Are you working on it? ― You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#21 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AH3QTF4YHLYRCFDQK2CG5Y3TSBPXDANCNFSM44YIOD6Q>.

mmusich · 2021-06-10T17:29:21Z

@gu180 please start from the version of this branch, I substantially streamlined the configuration part in c13317c

connorpa · 2021-06-11T06:42:35Z

Alignment/OfflineValidation/bin/Zmumumerge.cc

+#include <boost/filesystem.hpp>
+#include <boost/property_tree/ptree.hpp>
+#include <boost/property_tree/json_parser.hpp>
+#include <boost/optional.hpp>


I don't see anywhere where you are using boost::optional: why do you load the header?

This comment may apply to RooFit headers are well -- please check.

I have deleted it.

in the last push I've taken the latest version from https://github.com/gu180/TrackderAlignment_DiMuonValidation/tree/all-in-one.
I am not sure all the comments have been addressed.
Unfortunately since apparently @gu180 is using dos formatting while everybody else is using unix, the comparison gets spoiled as the entire file looks changed.

I have renewed the DiMuonValidation.cc file. Is there still dos and unix problem?

connorpa · 2021-06-11T06:45:13Z

Alignment/OfflineValidation/bin/Zmumumerge.cc

+  FitOut(double a, double b, double c, double d) : mean(a), mean_err(b), sigma(c), sigma_err(d) {}
+};
+
+FitOut ZMassBinFit_OldTool(TH1D* th1d_input, TString s_name = "zmumu_fitting", TString output_path = "./") {


Why OldToot? This naming convention is unclear.

Also, it could be of advantage to write comments à la doxygen to explain what each function is doing.

connorpa · 2021-06-11T06:45:24Z

Alignment/OfflineValidation/bin/Zmumumerge.cc

+  fitter->fit(th1d_input, "breitWignerTimesCB", "exponential", xMin, xMax, false);
+
+  c1->Print(Form("%s/fitResultPlot/%s_oldtool.pdf", output_path.Data(), s_name.Data()));
+  c1->Print(Form("%s/fitResultPlot/%s_oldtool.root", output_path.Data(), s_name.Data()));


The fitting of mumu invariant mass spectrum is from the old Zmumu tool.

Maybe but this is not a proper naming convention. If the function is being actively used, then just remove _oldtool. If the function is no longer used, remove it from the code.

connorpa · 2021-06-11T06:52:08Z

Alignment/OfflineValidation/bin/Zmumumerge.cc

+  }
+
+  gSystem->Exec(Form("mkdir -p %s", output_path.Data()));
+  gSystem->Exec(Form("mkdir -p %s/fitResultPlot", output_path.Data()));


It would be better to use the filesystem library to create directories and handle files in general. You are actually loading the header of boost::filesystem. Here it would be sth like:

using fs = boost::filesystem; // after the headers // ... fs::path p = output_path.Data(); p /= "fitResultPlot"; fs::create_directories(p); assert(fs::exists(p));

connorpa · 2021-06-11T06:53:37Z

Alignment/OfflineValidation/bin/Zmumumerge.cc

+  vector<TString> vec_global_tag;
+  vec_single_file_path.clear();
+  vec_single_file_name.clear();
+  vec_global_tag.clear();


Why are you using clear right after declaring the vectors? Is there a particular reason?

Naïvely, I'd say that these lines aren't necessary, but I might be missing sth.

But maybe you don't need these vectors at all -- see comments below.

I have deleted these codes.

connorpa · 2021-06-11T06:57:14Z

Alignment/OfflineValidation/bin/Zmumumerge.cc

+  pt::read_json(options.config, main_tree);
+  pt::ptree alignments = main_tree.get_child("alignments");
+  pt::ptree validation = main_tree.get_child("validation");
+  for (const std::pair<std::string, pt::ptree>& childTree : alignments) {


You could just use auto here.

Also childTree is not a really good name. Why not calling it alignment?

connorpa · 2021-06-11T06:58:53Z

Alignment/OfflineValidation/bin/Zmumumerge.cc

+  pt::ptree validation = main_tree.get_child("validation");
+  for (const std::pair<std::string, pt::ptree>& childTree : alignments) {
+    vec_single_file_path.push_back(childTree.second.get<std::string>("file"));
+    vec_single_file_name.push_back(childTree.second.get<std::string>("file") + "/Zmumu.root");


I am not sure to understand why you declare vectors in which you simply copy the information of the ptrees. Why not using directly the content of the ptrees? Then you would have one loop less, and you would be able to pass additional options if this is requested from the JSON config file.

The function Draw_TH1D_forMultiRootFiles() need vectors as input.

Exactly: please change this function too, so that you don't have to convert a ptree to a vector.

connorpa · 2021-06-11T06:59:02Z

Alignment/OfflineValidation/bin/Zmumumerge.cc

+#ifndef DOXYGEN_SHOULD_SKIP_THIS
+int main(int argc, char* argv[]) {
+  return Zmumumerge(argc, argv);
+  //return Zmumumerge(argc, argv);


//return Zmumumerge(argc, argv);
This is deleted.

connorpa · 2021-06-11T06:59:46Z

Alignment/OfflineValidation/bin/Zmumumerge.cc

+using namespace AllInOneConfig;
+namespace pt = boost::property_tree;
+static const int colors_array[] = {4,  8,  2,  3,  4,  7,  30, 6,  9,  46, 36, 8,  10, 11,
+                                   12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24};


The colours (and the style options in general) should be retrieved from the JSON file. You don't need such an array here.

This has been solved, now the color and markerstyle is retrieved from the JSON file.
https://github.com/gu180/TrackderAlignment_DiMuonValidation/tree/all-in-one

connorpa

@gu180 I have reviewed a few things in the code directly, as suggested by @musich

Please take a look, thanks.

connorpa · 2021-06-11T07:00:24Z

Alignment/OfflineValidation/bin/Zmumumerge.cc

+  th1d_input[0]->SetTitle("");
+  for (int idx_file = 0; idx_file < file_number; idx_file++) {
+    th1d_input[idx_file]->SetMarkerColor(colors_array[idx_file]);
+    th1d_input[idx_file]->SetLineColor(colors_array[idx_file]);


As said above: don't use a fixed scheme for the colours. Use the options in the JSON file.

I have modified the code so that the color and markerstyle can be setted in the JSON file.
https://github.com/gu180/TrackderAlignment_DiMuonValidation/tree/all-in-one

connorpa · 2021-06-11T07:01:49Z

Alignment/OfflineValidation/python/TkAlAllInOneTool/Zmumu_cfg.py

+###################################################################
+# The Di Muon Mass Validation module
+###################################################################
+process.DiMuonMassValidation = cms.EDAnalyzer('DiMuonValidation',


In general, here, it would be worthwhile to give the possibility to the lambda user to override all the following options from the config file. The arguments shouldn't be mandatory, so if the value exists, use it, otherwise use the default value.

I do not quite understand this comment. Do you mean change those values to something like this:
cms.untracked.int32(config["validation"].get("maxevents", 2000000)) ?

That's the idea: don't hardcode the value, but give the possibility to the aligner to overwrite these values from the config file (while keeping a default value if he does not want to overwrite it). Take a look at the implementation of other validations.

These are physical geometrical boundaries. Was the Z->mm validation ever run with partial coverage of the phase-space ?
In my opinion only the number of bins could make sense to be configurable. Too much freedom to the users quickly leads to nonsense.

In the past we did it.

Anyway, the smart aligner will know where to change the value if necessary. I agree that it is an exotic case, so ok if you prefer fixing it once for all.

However, the number of bins is certainly sth needed indeed.

In the past we did it.

I think what was done in the past was to limit the acceptance of the single muon track to disentangle barrel from endcaps, not to plot the mass bias only up to some η or φ value. The two things are different.
By the way, the current code doesn't provide for the former functionality.

connorpa · 2021-06-11T07:02:55Z

Alignment/OfflineValidation/test/testZmumu.yaml

+    Zmumu:
+        merge:
+            Legacy:
+                methods:


Where is this option used in your code? I haven't found it.

Check this for all options: it would make no sense to give options that aren't used...

mmusich · 2021-06-16T07:08:05Z

in the last push I've taken the latest version from https://github.com/gu180/TrackderAlignment_DiMuonValidation/tree/all-in-one.
I am not sure all the comments have been addressed.
Unfortunately since apparently @gu180 is using dos formatting while everybody else is using unix, the comparison gets spoiled as the entire file looks changed.

mmusich · 2021-07-15T07:53:27Z

@gu180 @connorpa have all the comments been addressed? when can we merge this?

connorpa · 2021-07-15T09:38:32Z

@gu180 @connorpa have all the comments been addressed? when can we merge this?

IMHO things could still be improved, but I don't think that the comments are critical. If time is running out, we can proceed with the code as it is.

mmusich · 2023-01-09T11:01:45Z

Integrated in main cmssw fork via cms-sw#40346

mmusich added 2 commits May 11, 2021 14:13

ZMuMu validation in all-in-one tool from An Gu

97ee064

apply code-checks and code-format

8564db4

mmusich requested review from DaveBrun94, connorpa and henriettepetersen May 12, 2021 10:02

clean-up analyzer and cmssw python configuration

c13317c

Merge branch 'newAllInOneTool' into zMMInToNewAllInOneTool

ad17b94

connorpa reviewed Jun 11, 2021

View reviewed changes

code review, from @gu180: 16.08.2021

8d62418

mmusich mentioned this pull request Dec 16, 2022

Add DiMuonValidation to the TkAl "new" all-in-one tool cms-sw/cmssw#40346

Merged

mmusich closed this Jan 9, 2023

[WIP] Z->mm in to new all in one tool #21

[WIP] Z->mm in to new all in one tool #21

Uh oh!

Conversation

mmusich commented May 12, 2021

Uh oh!

connorpa commented May 26, 2021

Uh oh!

mmusich commented May 26, 2021

Uh oh!

connorpa commented Jun 10, 2021

Uh oh!

mmusich commented Jun 10, 2021

Uh oh!

gu180 commented Jun 10, 2021 via email

Uh oh!

mmusich commented Jun 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorpa Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorpa Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorpa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gu180 Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmusich commented Jun 16, 2021

connorpa Jun 11, 2021 •

edited

Loading

connorpa Jun 11, 2021 •

edited

Loading

gu180 Jun 15, 2021 •

edited

Loading