How to accumulate histogram using function

Newbie
1

Hi Experts,

I’m writing a function that accumulates histogram read from files to an empty histogram.

The function function that read the histograms and accumulate the histogram is as follow:

int CollectHist (
		TH1D *hist1d_input,
		int ncummulative,
		string name_hist,
		string path_input)
{
	TFile *file_input = new TFile (path_input.data(), "read");

	TH1D *hist1d_tmp = (TH1D*)file_input -> Get (name_hist.data());

	if (ncummulative<=0)
	{
		hist1d_input = (TH1D*)hist1d_tmp -> Clone();
	}
	else
	{
		hist1d_input -> Add (hist1d_tmp);
	}

	file_input -> Close();
	delete hist1d_tmp;
	delete file_input;

	return 1;
}

The main function the call CollectHistogram is:

void example ()
{
	TH1::AddDirectory(kFALSE);

	TH1D *hist;
	int ncum = 0;

	string list_file[3] =
	{
		"file1.root",
		"file2.root",
		"file3.root"
	};

	for (int i=0; i<3; i++)
	{
		int stt = CollectHist (
				hist,
				ncum,
				"histname",
				list_file[i]);
		ncum += stt;
	}
}

I got “segmentation fault” when I ran the code, as my hist became null again after the function had exited. It seem that I didn’t declare the argument in the function correctly

How can I make it work?

Thank you very much,
Hoa.

2

Hi @LongHoa ,

sorry for the high latency!

Is there a stacktrace printed at the point of crash? What does it say? Can you isolate the line that crashes?

If not, could you please also provide input files that we can use to run your code and debug?

Cheers,
Enrico

3

Hi Enrico,

I added a few lines to print the integral of the histogram I want to accumulate inside and after the CollectHist function. The one inside the function works, the one after function break the program with segmentation violation

Here is the full log:

Processing example.C...
 [+] Read files
  |-- File number 1

 *** Break *** segmentation violation
  |   |-- Integral before exist: 5000.00



===========================================================
There was a crash.
This is the entire stack trace of all threads:
===========================================================
#0  0x00007ffa48dd8337 in __GI___waitpid (pid=3993, stat_loc=stat_loc
entry=0x7fff8981b868, options=options
entry=0) at ../sysdeps/unix/sysv/linux/waitpid.c:30
#1  0x00007ffa48d43047 in do_system (line=<optimized out>) at ../sysdeps/posix/system.c:149
#2  0x00007ffa499810f3 in TUnixSystem::StackTrace() () from /home/longhoa/ROOT_Source/ROOT_v06_24_00/lib/libCore.so.6.24
#3  0x00007ffa49983be5 in TUnixSystem::DispatchSignals(ESignals) () from /home/longhoa/ROOT_Source/ROOT_v06_24_00/lib/libCore.so.6.24
#4  <signal handler called>
#5  0x00007ffa4a04d64d in ?? ()
#6  0x0000000000000000 in ?? ()
===========================================================


The lines below might hint at the cause of the crash.
You may get help by asking at the ROOT forum https://root.cern.ch/forum
Only if you are really convinced it is a bug in ROOT then please submit a
report at https://root.cern.ch/bugs Please post the ENTIRE stack trace
from above as an attachment in addition to anything else
that might help us fixing this issue.
===========================================================
#5  0x00007ffa4a04d64d in ?? ()
#6  0x0000000000000000 in ?? ()
===========================================================

I have several simple input files for testing:
file1.root (3.5 KB)
file2.root (3.6 KB)
file3.root (3.5 KB)

The code I full use with the added printf command:
example.C (1.3 KB)

and the code that generate the input root files above:
histgenerator.C (515 Bytes)

Thank you for your response,

Hoa.

4

Hi @LongHoa ,

thank you for sharing a simple reproducer.

From that code I assume you expect CollectHist to modify the contents of the hist pointer, but that’s not how it works. CollectHist actually works on its own local variable, hist1d_input, that is completely independent of the variable in the outer scope, hist (except for the fact that hist1d_input is initialized with the same value as hist). So when you modify hist1d_input inside CollectHist, that leaves hist unchanged and when you use hist afterwards you are accessing an invalid pointer (with undefined content as you never initialized the variable).

I hope this clarifies things.
Cheers,
Enrico

5

Hi Enrico,

Thank you for your reply.

From that code I assume you expect CollectHist to modify the contents of the hist pointer

yes, that is what I expect the code to work.

I figured that if I declare hist as a global variable and remove hist1d_input from the arguments, then the function will modify hist directly and I get my expected result.

I’m just curious if there is a way to make CollectHist modify the TH1D hist as expected, without using global variable?

Best,
Hoa

6

Yes, there are a couple of ways. The most straightforward is to pass hist by reference instead of by copy (CollectHist(TH1D *&hist1d_input). However the nicer design IMHO would be to avoid output parameters and instead do something like:

TH1D *AddHists(const char *histName,
               const std::vector<std::string> &files) {
   std::unique_ptr<TFile> file0{TFile::Open(files[0].c_str())};
   TH1D *h = file0->Get<TH1D>(histName)->Clone();
   for (auto fname = files.begin() + 1; fname != files.end(); ++fname) {
       std::unique_ptr<TFile> f{TFile::Open(fname->c_str())};
       h->Add(f->Get<TH1D>(histName));
   }
   return h;
}

int main() {
  TH1D* hist = AccumulateHists("histname", {"file1.root", "file2.root"});
}

I have not tested the code but I hope it conveys the idea. In general it’s a good practice to write functions that take some input, return some output and don’t otherwise modify the state of the program.

Cheers,
Enrico

1 Like
7

Thank you very much! Your solutions solves my problem.