For pilot2 d by lauraser · Pull Request #859 · AliceO2Group/QualityControl

lauraser · 2021-10-04T17:50:32Z

The following check takes a canvas as input, loops over all figures containing IROCs and OROCs in the canvas and checks for empty pads.
Besides storing the main quality object, it has private member to store qualities for every histogram which are later used in beautify to show specific quality for every histogram instead of the entire canvas.
@wiechula Stefan suggested the limits for medium quality 30% of pads empty and for bad 60% of pads empty. Do you think this is reasonable?

Barthelemy · 2021-10-05T07:20:20Z

I let @wiechula review this PR.

wiechula

@lauraser , thanks for the code.
I have many small comments to improve code readability mainly. For the future, in general, please always

use const whenever possible
always use readability braces

wiechula · 2021-10-05T08:03:35Z

+      if (titleh.find("IROC") != std::string::npos)
+        iroc = true;
+      if (titleh.find("OROC") != std::string::npos)
+        oroc = true;
+      if (!(iroc || oroc))
+        return Quality::Null;


add readability braces and simplify expression (assuming it cannot be both, IROC and OROC). Also, the IROC / OROC distinction is only used to get the total number of pads.

Suggested change

if (titleh.find("IROC") != std::string::npos)

iroc = true;

if (titleh.find("OROC") != std::string::npos)

oroc = true;

if (!(iroc || oroc))

return Quality::Null;

int totalPads = 0;

if (titleh.find("IROC") != std::string::npos) {

totalPads = 5280;

} else if (titleh.find("OROC") != std::string::npos) {

totalPads = 9280;

} else {

return Quality::Null;

}

wiechula · 2021-10-05T08:04:35Z

+      auto padName = fmt::format("c_ROCs_Pedestal_2D_{:d}", tpads);
+      auto histName = fmt::format("h_Pedestals_ROC_{:02d}", tpads - 1);


Suggested change

auto padName = fmt::format("c_ROCs_Pedestal_2D_{:d}", tpads);

auto histName = fmt::format("h_Pedestals_ROC_{:02d}", tpads - 1);

const auto padName = fmt::format("c_ROCs_Pedestal_2D_{:d}", tpads);

const auto histName = fmt::format("h_Pedestals_ROC_{:02d}", tpads - 1);

wiechula · 2021-10-05T08:08:37Z

+  Quality result = Quality::Null;
+  if (mo->getName() == "c_ROCs_Pedestal_2D") {
+    result = Quality::Good;
+    auto* canv = dynamic_cast<TCanvas*>(mo->getObject());


dynamic cast only makes sense if you want the verify the object type. So I guess you should either change this to static_cast of c-style cast (TCanvas*) or add

if (!canv) { return Quality::Null; }

wiechula · 2021-10-05T08:09:39Z

+      auto histName = fmt::format("h_Pedestals_ROC_{:02d}", tpads - 1);
+      TPad* pad = (TPad*)canv->GetListOfPrimitives()->FindObject(padName.data());
+      TH2F* h = (TH2F*)pad->GetListOfPrimitives()->FindObject(histName.data());
+      std::string titleh = h->GetTitle();


Suggested change

std::string titleh = h->GetTitle();

const std::string titleh = h->GetTitle();

wiechula · 2021-10-05T08:13:54Z

+      int totalPads = 0;
+      if (iroc)
+        totalPads = 5280;
+      if (oroc)
+        totalPads = 9280;


Not needed, see above.

Suggested change

int totalPads = 0;

if (iroc)

totalPads = 5280;

if (oroc)

totalPads = 9280;

wiechula · 2021-10-05T08:17:52Z

+    auto padName = fmt::format("c_ROCs_Pedestal_2D_{:d}", tpads);
+    auto histName = fmt::format("h_Pedestals_ROC_{:02d}", tpads - 1);


Suggested change

auto padName = fmt::format("c_ROCs_Pedestal_2D_{:d}", tpads);

auto histName = fmt::format("h_Pedestals_ROC_{:02d}", tpads - 1);

const auto padName = fmt::format("c_ROCs_Pedestal_2D_{:d}", tpads);

const auto histName = fmt::format("h_Pedestals_ROC_{:02d}", tpads - 1);

wiechula · 2021-10-05T08:18:11Z

+    TPad* pad = (TPad*)canv->GetListOfPrimitives()->FindObject(padName.data());
+    pad->cd();
+    TH2F* h = (TH2F*)pad->GetListOfPrimitives()->FindObject(histName.data());
+    std::string titleh = h->GetTitle();


Suggested change

std::string titleh = h->GetTitle();

const std::string titleh = h->GetTitle();

wiechula · 2021-10-05T08:18:31Z

+    if (it != badSectorsName.end()) {
+      TPaveText* msg = new TPaveText(0.1, 0.9, 0.9, 0.95, "NDC");
+      msg->SetBorderSize(1);
+      int index = std::distance(badSectorsName.begin(), it);


Suggested change

int index = std::distance(badSectorsName.begin(), it);

const int index = std::distance(badSectorsName.begin(), it);

wiechula · 2021-10-05T08:20:25Z

+    pad->cd();
+    TH2F* h = (TH2F*)pad->GetListOfPrimitives()->FindObject(histName.data());
+    std::string titleh = h->GetTitle();
+    std::vector<std::string>::iterator it = std::find(badSectorsName.begin(), badSectorsName.end(), titleh);


Suggested change

std::vector<std::string>::iterator it = std::find(badSectorsName.begin(), badSectorsName.end(), titleh);

auto it = std::find(badSectorsName.begin(), badSectorsName.end(), titleh);

wiechula · 2021-10-05T08:21:11Z

+    TH2F* h = (TH2F*)pad->GetListOfPrimitives()->FindObject(histName.data());
+    std::string titleh = h->GetTitle();
+    std::vector<std::string>::iterator it = std::find(badSectorsName.begin(), badSectorsName.end(), titleh);
+    if (it != badSectorsName.end()) {


Change logic to make clear that the reader does not have to continue reading the code:

Suggested change

if (it != badSectorsName.end()) {

if (it == badSectorsName.end()) {

continue

}

Barthelemy · 2021-10-06T06:35:53Z

error unrelated, merging

wiechula · 2021-10-06T07:19:43Z

@Barthelemy , seem it is not merged.

lauraser requested a review from wiechula as a code owner October 4, 2021 17:50

lauraser force-pushed the ForPilot2D branch from 48a92b7 to ea0cceb Compare October 5, 2021 07:30

wiechula requested changes Oct 5, 2021

View reviewed changes

lauraser requested review from Barthelemy, knopers8, mfasDa and njacazio as code owners October 5, 2021 20:18

lauraser force-pushed the ForPilot2D branch 2 times, most recently from e3ca053 to 9a71874 Compare October 5, 2021 20:30

The 2D PadCalibragion check

69d8370

lauraser force-pushed the ForPilot2D branch from 9a71874 to 69d8370 Compare October 5, 2021 20:31

wiechula approved these changes Oct 5, 2021

View reviewed changes

Check if pads and histos not empty

68b62ee

wiechula approved these changes Oct 5, 2021

View reviewed changes

Barthelemy merged commit 2cf5594 into AliceO2Group:master Oct 6, 2021

		auto padName = fmt::format("c_ROCs_Pedestal_2D_{:d}", tpads);
		auto histName = fmt::format("h_Pedestals_ROC_{:02d}", tpads - 1);

	std::string titleh = h->GetTitle();
	const std::string titleh = h->GetTitle();

	int index = std::distance(badSectorsName.begin(), it);
	const int index = std::distance(badSectorsName.begin(), it);

	std::vector<std::string>::iterator it = std::find(badSectorsName.begin(), badSectorsName.end(), titleh);
	auto it = std::find(badSectorsName.begin(), badSectorsName.end(), titleh);

-    if (it != badSectorsName.end()) {
+    if (it == badSectorsName.end()) {
+      continue
+    }

Conversation

lauraser commented Oct 4, 2021

Uh oh!

Barthelemy commented Oct 5, 2021

Uh oh!

wiechula 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!

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!

Barthelemy commented Oct 6, 2021

Uh oh!

wiechula commented Oct 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants