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

[WIP] TPC QC: adds occupancy histogram in Cluster task #2124

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sbhawani
Copy link
Contributor

@sbhawani sbhawani commented Feb 1, 2024

No description provided.

@sbhawani sbhawani requested a review from wiechula as a code owner February 1, 2024 16:49
processClusterNative(ctx.inputs());
processKrClusters(ctx.inputs());

if (!mIsMergeable) {
mQCClusters.getClusters().normalize();
mQCClusters.getClusters().normalize(grpECS->getNHBFPerTF());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not needed, nHBF is already set above.

Suggested change
mQCClusters.getClusters().normalize(grpECS->getNHBFPerTF());
mQCClusters.getClusters().normalize();

@@ -65,6 +65,12 @@ class ClusterVisualizer final : public quality_control::postprocessing::PostProc
/// \param services Interface containing optional interfaces, for example DatabaseInterface
void finalize(quality_control::postprocessing::Trigger, framework::ServiceRegistryRef) override;

template <class T>
void makeRadialProfile(o2::tpc::CalDet<T>& calDet, TCanvas* canv, int nbinsY, float yMin, float yMax);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most probably these functions will never be called outside this code. So the can be in the private part of the class.
Also, the binning is not needed here, since it is only taken from mRanges via the calDet name. Also, the object should not be modified, so it can be `const.

Suggested change
void makeRadialProfile(o2::tpc::CalDet<T>& calDet, TCanvas* canv, int nbinsY, float yMin, float yMax);
void makeRadialProfile(const o2::tpc::CalDet<T>& calDet, TCanvas* canv);

Comment on lines 212 to 219

calDet = clusters.getOccupancy();
vecPtr = toVector(mCalDetCanvasVec.at(calDetIter));
o2::tpc::painter::makeSummaryCanvases(calDet, int(mRanges[calDet.getName()].at(0)), mRanges[calDet.getName()].at(1), mRanges[calDet.getName()].at(2), false, &vecPtr);
calDetIter++;
vecPtr = toVector(mCalDetCanvasVec.at(calDetIter));
makeRadialProfile(calDet, vecPtr.at(0), int(mRanges[calDet.getName()].at(0)), mRanges[calDet.getName()].at(1), mRanges[calDet.getName()].at(2));
calDetIter++;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is a problem in the logic, since calDet is a reference to the NClusters cal det. So all subsequent assignments will overwrite the NClusters calDet. Also, there is a lot of unnecessary repetitive code here. I propose to add a simple lambda and replace everything below

auto& clusters = clusterData->getClusters();

with

auto fillCanvases = [&calDetIter, this](const CalDet& calDet) {
  auto vecPtr = toVector(mCalDetCanvasVec.at(calDetIter++));
  const auto& ranges = mRanges[calDet.getName()];
  o2::tpc::painter::makeSummaryCanvases(calDet, int(ranges.at(0)), ranges.at(1), ranges.at(2), false, &vecPtr);
};

  fillCanvases(clusters.getNClusters());
  fillCanvases(clusters.getQMax());

  if (mIsClusters) {
    fillCanvases(clusters.getQTot());
    fillCanvases(clusters.getSigmaPad());
    fillCanvases(clusters.getSigmaTime());
  }

  fillCanvases(clusters.getTimeBin());
  fillCanvases(clusters.getOccupancy());

  makeRadialProfile(calDet, mCalDetCanvasVec.at(calDetIter++).at(0).get());

This should be much more compact and easier to read.

Comment on lines 232 to 227
void ClusterVisualizer::makeRadialProfile(o2::tpc::CalDet<T>& calDet, TCanvas* canv, int nbinsY, float yMin, float yMax)
{
const std::string_view calName = calDet.getName();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
void ClusterVisualizer::makeRadialProfile(o2::tpc::CalDet<T>& calDet, TCanvas* canv, int nbinsY, float yMin, float yMax)
{
const std::string_view calName = calDet.getName();
void ClusterVisualizer::makeRadialProfile(const o2::tpc::CalDet<T>& calDet, TCanvas* canv)
{
const std::string_view calName = calDet.getName();
const auto& ranges = mRanges[calName.data()];
const int nbinsY = int(ranges.at(0));
const float yMin = ranges.at(1);
const float yMax = ranges.at(2);

@wiechula
Copy link
Collaborator

@sbhawani , please fix the clang-format

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants