Weird thing with histogram and directories

Hi,

The following smells like a bug to me. I have a simple program:

#include <TH2.h>
#include <TFile.h>
#include <iostream>

using namespace std;

int main(int argc, char* argv[]) {
  TH2D h2;
  TH2D h;

  TFile f("some_file.root","RECREATE");

  cout << h.GetDirectory() << endl;
  cout << h2.GetDirectory() << endl;
  h2 = h;
  cout << h2.GetDirectory() << endl;

  f.Close();
  return 0;
}

and the output of this program is:

It seems to me that, since I am not instanciating a new histogram after the file was open, h2’s directory should be the same as h’s but it isn’t.

I guess the last line appears when the destructor for h2 is called at the end. It does not appear if I set h2’s directory to 0 (as it should be) before closing the file.

Using root 5.22/00.

Correct me if I am wrong but h2 and h are not pointers and therefore:

h2 = h; // new histogram is created here in the current directory through operator=

and current directory happens to be inside of newly opened TFile.

[quote=“ardashev”]Correct me if I am wrong but h2 and h are not pointers and therefore:

h2 = h; // new histogram is created here in the current directory through operator=[/quote]

Wrong: h2’s operator = is called and that sets the data members inside h2, an already existing instance. No new instance is created.

There are two “sensible” solutions. That h2’s directory does not change or that it is set to h’s but none of them is true in this case.

Wrong wrong :slight_smile:
theoretically You are right, but ROOT actually creates a copy of histogram
by default in current directory.

( by following the white rabbit into source code of operator= I find:)
if (fgAddDirectory && gDirectory) {
gDirectory->Append(&obj);
((TH1&)obj).fDirectory = gDirectory;

This is more of a philosophical discussion at this point whether this is bug or not.
For my taste and from standard of C++ it IS BUG, but then it will probably break some other conventional rules of root storage schemes in some other places.

You have 2 options: file a bug report and see what developers say OR
just remember that any copy operation creates new histogram in CURRENT directory.

There is of course option to change fgAddDirectory = kFALSE, but it is probably more confusing in the long run:-).

BTW nice catch.

[quote]It seems to me that, since I am not instanciating a new histogram after the file was open, h2’s directory should be the same as h’s but it isn’t. [/quote]However, when held in the list of object of a TDirectory the histograms are retrieved by name (you may have seen the related warning about a memory leak if you re-use the same name for an histogram).
So since histogram ‘names’ are supposed to be unique in each directory, we assume that you do not want the copy to hide the original and hence attach the copy to the current directory. (admittedly in your use case this is a bit counter intuitive).

If you need a different behavior either cancel the automatic attachment of the histogram to the directories (TH1::AddDirectory(kFALSE):wink: or (always) explicitly set the directory. For example you can get exactly the behavior you wanted by doing:

[code]int main(int argc, char* argv[]) {
TH2D h2;
TH2D h;

TFile f(“some_file.root”,“RECREATE”);

cout << h.GetDirectory() << endl;
cout << h2.GetDirectory() << endl;
h2 = h;
h2.SetDirectory( h.GetDirectory() );
cout << h2.GetDirectory() << endl;

f.Close();
return 0; [/code]

Cheers,
Philippe.

Hi,

This is my old post (one year old!) so I decided to celebrate the aniversary by commenting that I am puzzled that you do not consider this a bug. I think it is a very big issue.

In a small program this is no problem but in a bigger program, consisting of many classes, it could happen (and it happened to me) that I create a histogram file in one class, open a file in another and then somehow assign it somewhere else. In a framework, there is absolutely no way to enforce something like TH1::AddDirectory(kFALSE)
and the only option I am left with is to do what you say:

h2 = h; h2.SetDirectory( h.GetDirectory() );
in every single assignment and this is what I ended up doing.

To me, this is not a good idea. Maybe I am too close minded? :wink:

I am afraid that I do not have a positive answer. When you say:

[quote]I create a histogram file in one class, open a file in another and then somehow assign it somewhere else.
[/quote]You are describing yourself the mess that you are introducing in your system. It would be nice to have a clear description of the problem that you are trying to solve. We will see if we can help.

Rene

Hi,

or simply put TH1::AddDirectory(false) into your rootlogon.C file.

Cheers, Axel.

Hi Rene and Alex,

Thanks for responding. Only now I see this (old) reply to my (old) post.

There was no mess on my part. It was just that I was programming thinking that someone would use the tool I was writing. Then I realized that Root is just not meant for that, because it is intrinsically unsafe and I did something else. I mean: I can’t tell everyone who uses whatever I write that they have to do TH1::AddDirectory(false) in their rootlogon.C.

Apart from the fact that most of the bugs I have found in my code were related to Root’s memory management, and that I do not agree with this global behaviour, I do think that what I pointed out is a fundamental problem in the assignment operator. The piece of code I copy/pasted used to crash (I haven’t tried again). You want a clearer description than that?

Don’t you see that the ownership is changed inside the assignment operator? Even when I explicitly set the directory to 0 in both histograms before hand???

Again, thanks for replying. I will not post in this discussion anymore.

I’m with javiergt on this one. Peppering h.SetDirectory(0) throughout the code is getting pretty annoying.

The fundamental issue here is that the assignment operator comes with the assumption that the objects will be identical after the assignment (up to the location of allocated memory). If this is not the case, there needs to at least be a warning flag when you make the assignment or in the TH1::operator=() documentation.

This issue still exists in 5.32/04.

I’m not sure I’d call it a “very big” issue, but it is an easy way to lose an hour chasing the bug.