GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — [WIP] TPC QC: adds occupancy histogram in Cluster task by sbhawani · Pull Request #2124 · AliceO2Group/QualityControl

Conversation

sbhawani commented Feb 1, 2024

Copy link

Copy Markdown

Contributor

No description provided.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

sbhawani requested a review from wiechula as a code owner

February 1, 2024 16:49

wiechula requested changes

Feb 5, 2024

View reviewed changes

Comment thread

Modules/TPC/src/Clusters.cxx Outdated


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

wiechula Feb 5, 2024

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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();

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/TPC/include/TPC/ClusterVisualizer.h Outdated

		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);

wiechula Feb 5, 2024

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/TPC/src/ClusterVisualizer.cxx Outdated

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++;

wiechula Feb 5, 2024

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/TPC/src/ClusterVisualizer.cxx Outdated

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();

wiechula Feb 5, 2024

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

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);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tklemenz and others added 2 commits

April 18, 2024 17:48


          TPC: add occupancy plot to Cluster task

76a92df


          Merge branch 'master' into ClusterQC

fa401f7

sbhawani force-pushed the ClusterQC branch from ba1b98e to fa401f7 Compare

May 15, 2024 09:20

wiechula commented May 16, 2024

Copy link

Copy Markdown

Collaborator

@sbhawani , please fix the clang-format

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

justonedev1 commented Mar 12, 2025

Copy link

Copy Markdown

Collaborator

@wiechula @sbhawani is this PR still relevant?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ariedel-cern mentioned this pull request

May 8, 2025

TPC QC: adds occupancy histogram in Cluster task #2551

Merged

This branch has not been deployed

No deployments

Reviewers

wiechula

wiechula requested changes

Requested changes must be addressed to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.