Add compare macro

[fuhlig1]

Add a macro which allows to compare the cbmsim tree of two different ROOT macros. The added script does this comparison for all relevant ROOT files produced by the test suite. This allows to easily compare the results before and after a change of FairRoot to see if there are unexpected changes.

Checklist:

• [ x] Followed the Contributing Guidelines

[ChristianTackeGSI]

First round of comments.

Could we please get a test for this tooling somehow?

Maybe we could integrate it in some other test by running the simulation with two seeds and then compare the results? (which should hopefully be quite the same?)

[fuhlig1]

The test will be the next step. First I want to get the macro and the script in which are the prerequisites for the test.
I will look for the comments probably only next week.

[dennisklein]

We should generate this list somehow, having it hardcoded like this will be a nightmare to maintain.

[fuhlig1]

I agree but I am not sure if this will be so easy. For the time being it would be good if we could keep the hard coded list.

[dennisklein]

An idea would be to glob for ${dir1}/**/*.root and then iterate this list of files, if such a root file also exists in $dir2. We may have to add then another sanity check to the TreeCompareAuto.C macro that it will skip non-FairRoot root files, if there were any.

[fuhlig1]

The problem is that not all ROOT files should be tested. Not all off them contain the tree. That means that we have to filter the list afterwards to get only the list of files we want. In my opinion this isn't better than having a fixed list.

One solution would be to provide some benchmark files you want to compare and loop over these files and compare them with the newly produced ones. The downside of this is that we have to provide the benchmark files somehow.

[dennisklein]

Another approach would be:

• Add a CMake list variable FAIRROOT_SIMTREE_FILES_FOR_INTEGRITY_TESTING
• Append file names to it in each of the local CMakeLists.txt files from each example/test the file belongs to
• Then configure_file() this checkfiles.sh script in a way that the contents of the CMake variable is added

Then the "authority" of naming relevant root files stays local to the individual example/test/lib. What do you think?

[fuhlig1]

@ChristianTackeGSI, @dennisklein,

thanks for all the suggestions. I hope I addressed them now all.

[dennisklein]

Suggested change

TCanvas c1;

and see following comment further down

[fuhlig1]

This is needed to suppress a warning/info from ROOT. I add a comment and renamed the variable.

[dennisklein]

Suggested change

	TString command1 = leafName + ">>htemp";
	tree1->Draw(command1);
	int entries{0};
	float low{0.};
	float high{0.};
	int nBins{0};
	auto htemp = static_cast<TH1F*>(gPad->GetPrimitive("htemp"));
	if (htemp) {
	entries = htemp->GetEntries();
	nBins = htemp->GetNbinsX();
	low = htemp->GetXaxis()->GetXmin();
	high = htemp->GetXaxis()->GetXmax();
	}
	TH1F htemp;
	long long entries = tree1->GetEntries();
	float value = 0;
	tree1->SetBranchAddress(leafName, &value);
	for (long long i = 0; i < entries; ++i) {
	tree1->GetEntry(i);
	htemp->Fill(value);
	}
	float low = htemp.GetXaxis()->GetXmin();
	float high = htemp.GetXaxis()->GetXmax();
	int nBins = htemp.GetNbinsX();

Why not something simple like this (have not tested the above code, but there should be some code similar to the above)?

What is the benefit to go through all this Draw() logic and global referencing via gPad?

We could also get rid of the htemp->Clear(); etc?

[dennisklein]

For the hist case a bit further down, one could use a TFormula, but since it is just a subtraction, not even needed?

[fuhlig1]

I think this will not work since the value can be anything and must not be a float.

[dennisklein]

Then why do you cast it to a TH1F* where the F stands for float? (Also, how are low and high then supposed to work if it is not a float?)

[dennisklein]

(Also, how are low and high then supposed to work if it is not a float?)

Btw, TAxis::GetXmax() returns a double?!

[dennisklein]

In our case, it should rather depend on the data in the root file, should it not?

[fuhlig1]

You can pass any number to TH1F::Fill() which will find the proper bin to increase. The type of the histogram only defines the maximum value which can be stored per bin. TH1C can only store values less than 128 per bin. TH1F stores the bin content in a float value.

These are the values you have to pass to the constructor which is the same for all types of one dimensional histograms

[in] | name | name of histogram (avoid blanks) | const char*
[in] | title | histogram title. | const char*
[in] | nbins | number of bins | int
[in] | xlow | low edge of first bin | double
[in] | xup | upper edge of last bin (not included in last bin) | double

TAxis::GetMax returns the value value of the upper edge of the last bin which is a double.

[dennisklein]

Ok, thx, this I misunderstood. But then the common type to fill the histogram with is just double? So, read the tree content into doubles (instead of float as I did in my suggestion)?

[dennisklein]

Or as a question: Which H1F API is used by TTree::Draw to add its stored values to the histogram?

[dennisklein]

And whatever the code looks like, my points are:

1. Why do we need to involve a TCanvas object, a TPad object and a function called Draw - all clearly graphics related entities - in a script that does not want to produce any graphics? Especially, if we have to create a random TCanvas object in the middle which is not used and just for some side effect? Don't you agree that this is a terrible program to write, read and maintain (source code readability AND computational complexity)?
2. Can we produce the histogram object without the indirection through the global object gPad?

[ChristianTackeGSI]

I didn't follow all the discussions. So please excuse, if I duplicate anything! (In that case, just write a short "dupe" and press the resolve button!)

[ChristianTackeGSI]

Can we please call this histogram hdifferences or something like that?

[ChristianTackeGSI]

Can this be replaced by a TRangeDynCast?
Something like this maybe?

for (auto key : TRangeDynCast<TKey const>(keylist)) {
    if (!key) {
        continue;
    }
    …
}

[dennisklein]

Suggested change

	int TreeCompareAuto(TString fileName1 = "", TString fileName2 = "")
	int TreeCompareAuto(TString fileName1, TString fileName2)

We anyways expect non-empty arguments.

[ChristianTackeGSI]

Hi @fuhlig1 could you consider adding a compile check for the macro like in #1421?

[ChristianTackeGSI]

As I recently learned, ReadObj returns an owning pointer.

[ChristianTackeGSI]

TKey::ReadObj returns an owning pointer (see #1461).

Maybe move this in front of the loop?

    std::unique_ptr<TTree> tree1;

And then:

Suggested change

	if (key->ReadObj()->InheritsFrom("TTree")) {
	treeName = key->GetName();
	std::cout << "Found TTree with name " << treeName << std::endl;
	numTreeInFile++;
	}
	tree1.reset(key->ReadObject<TTree>());
	if (tree1) {
	treeName = key->GetName();
	std::cout << "Found TTree with name " << treeName << std::endl;
	numTreeInFile++;
	}

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will not be closed.