Properly use `Copy(TObject &)` methods in `hist` classes #10942

linev · 2022-07-11T08:45:45Z

This is virtual method and can be overriden in derived classes.
Therefore when such method used in copy constructor or assignment
operator, one have to explicitly specify class which used.

Fixes #10919

One should explicitly specify which method is used

They should not be used at all

Use nullptr, remove dummy ";"

It is always dangerous because of potential linking problems

phsft-bot · 2022-07-11T08:45:53Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2022-07-11T09:22:13Z

Build failed on ROOT-ubuntu18.04/nortcxxmod.
Running on sft-ubuntu-1804-3.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments

phsft-bot · 2022-07-11T09:36:53Z

Build failed on ROOT-performance-centos8-multicore/cxx17.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

phsft-bot · 2022-07-11T09:42:01Z

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments

phsft-bot · 2022-07-11T10:09:53Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments

phsft-bot · 2022-07-11T10:09:56Z

Build failed on ROOT-debian10-i386/soversion.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments

phsft-bot · 2022-07-11T10:51:17Z

Build failed on mac1015/cxx17.
Running on macitois22.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments

phsft-bot · 2022-07-11T10:56:06Z

Build failed on mac11/cxx14.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments

lmoneta

I am not sure if the use of non-virtual Copy in the copy constructor of the histogram class is correct.
This PR will make a copy a TProfile to a TH1D possible which can have some side effects.
Example:

TProfile p("p","p",10,0,10);
p.Fill(0.5,10); p.Fill(0.5,20);
TH1D & h1 = p;
TH1D h2 = h1;
h2.Draw();
h1.SetLineColor(kRed);
h1.Draw("SAME");

This code works silently with this PR producing an histogram from a Profile but with wrong information (e.g. the bin error).
I don't see then the benefits of not using the virtual Copy in the copy constructor and assignment operator in the case of the histograms.

linev · 2022-08-03T04:35:35Z

If one want to keep usage of virtual Copy methods, in each such method one should check if source object type match this object - using dynamic cast. Otherwise it may produce segmentation violation.

In my mind, current implementation of copy constructors with using of virtual Copy methods is wrong and error prone.

lmoneta · 2022-08-03T10:29:00Z

Are you referring about a check that the final type of the source is the same final type pf the target, i.e. in this code below:

TH2D::TH2D(const TH2D &h2d) : TH2(), TArrayD()
 {
    ((TH2D&)h2d).Copy(*this);
 }

if this and 'h2dare actually the same final types (e.g. both TProfile histograms). This check is performed in theTProfile::Copy` function, see :
https://github.com/root-project/root/blob/master/hist/hist/src/TProfile.cxx#L422

linev · 2022-08-03T11:41:49Z

This check is performed in theTProfile::Copy` function,

Such check must be introduced in all Copy methods in all classes.
To avoid such checks, I propose directly call method of appropriate class and do not use virtual interface.

lmoneta · 2022-08-03T12:16:55Z

I agree if one is able to call copy constructors only for the final classes and not for base ones. In the ROOT histogram design this is not the case, since we have classes as TProfile deriving from TH1D and not TH1.

linev · 2022-08-03T21:27:23Z

When calling virtual Copy method in copy constructor - one never knows that exact happens.
User can define derived class from TH1D and provide own Copy implementation.
In such case TH1D th1 = user_th1; will leads to wrong results. TProfile is just example of that with existing ROOT classes.

These classes are base for TProfile[,2D,3D] and copy constructor should not be used together with such classes. This is check if TProfile::Copy() method.

phsft-bot · 2022-08-17T13:33:53Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2022-08-17T14:07:40Z

Build failed on ROOT-ubuntu18.04/nortcxxmod.
Running on sft-ubuntu-1804-2.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments

phsft-bot · 2022-08-17T14:12:38Z

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments

phsft-bot · 2022-08-17T14:36:37Z

Build failed on ROOT-performance-centos8-multicore/cxx17.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments

phsft-bot · 2022-08-17T15:08:25Z

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments

phsft-bot · 2022-08-17T15:38:13Z

Build failed on ROOT-debian10-i386/soversion.
Running on pcepsft11.dyndns.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments

phsft-bot · 2022-08-17T16:03:09Z

Build failed on mac11/cxx14.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.root.meta.roottest_root_meta_runMemberComments

lmoneta

Thank you Sergei for the improvement and the change in TH1,2,3,D classes.
It looks fine to me now.
I think there is a reference file that needs to be update in the roottest repo for avoiding the failure seen in the CI, after the change to use nullptr.

linev · 2022-08-17T17:21:31Z

I already prepare PR for root-test:

root-project/roottest#884

I will merge it shortly after merging this PR

phsft-bot · 2022-08-17T18:47:03Z

Build failed on mac1015/cxx17.
Running on macitois22.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2022-08-17T18:47:03.012Z] CMake Error at /Volumes/HD2/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1088 (message):

linev added 6 commits July 11, 2022 10:35

Correctly use Copy method in TF[1,2,3] functions classes

555cf93

One should explicitly specify which method is used

Call proper Copy method with TH[1,2,3] classes

b911700

Use proper Copy method in TProfile[,2D,3D] classes

4d6c180

Mark copy constructors for TH1, TH2, TH3 classes as deleted

ef27ab5

They should not be used at all

Improve syntax in TH1.h and TH2.h

f8a81bd

Use nullptr, remove dummy ";"

Move implementation of static methods from TH3.h

80c01d7

It is always dangerous because of potential linking problems

linev added bug affects:master labels Jul 11, 2022

linev requested a review from lmoneta as a code owner July 11, 2022 08:45

linev self-assigned this Jul 11, 2022

linev mentioned this pull request Jul 11, 2022

Histograms copy constructor has wrong implementation #10919

Closed

lmoneta reviewed Aug 2, 2022

View reviewed changes

Intentially call virtual Copy method in TH[1,2,3]D classes

3a20d7d

These classes are base for TProfile[,2D,3D] and copy constructor should not be used together with such classes. This is check if TProfile::Copy() method.

lmoneta approved these changes Aug 17, 2022

View reviewed changes

linev merged commit 0aba267 into root-project:master Aug 17, 2022

linev deleted the copy_hist branch August 17, 2022 17:23

guitargeek mentioned this pull request Dec 14, 2022

[RF] ProcessTimer for logging timings in RooFit::MultiProcess and RooFit::TestStatistics, and HeatmapAnalyzer for analysing them #11791

Merged

2 tasks

lmoneta mentioned this pull request Mar 3, 2023

Fix TEfficiency::Fit for 2D or 3D fits #12417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly use `Copy(TObject &)` methods in `hist` classes #10942

Properly use `Copy(TObject &)` methods in `hist` classes #10942

linev commented Jul 11, 2022

phsft-bot commented Jul 11, 2022

phsft-bot commented Jul 11, 2022

phsft-bot commented Jul 11, 2022

phsft-bot commented Jul 11, 2022

phsft-bot commented Jul 11, 2022

phsft-bot commented Jul 11, 2022

phsft-bot commented Jul 11, 2022

phsft-bot commented Jul 11, 2022

lmoneta left a comment •

edited

Loading

linev commented Aug 3, 2022

lmoneta commented Aug 3, 2022

linev commented Aug 3, 2022

lmoneta commented Aug 3, 2022 •

edited

Loading

linev commented Aug 3, 2022

phsft-bot commented Aug 17, 2022

phsft-bot commented Aug 17, 2022

phsft-bot commented Aug 17, 2022

phsft-bot commented Aug 17, 2022

phsft-bot commented Aug 17, 2022

phsft-bot commented Aug 17, 2022

phsft-bot commented Aug 17, 2022

lmoneta left a comment

linev commented Aug 17, 2022

phsft-bot commented Aug 17, 2022

Properly use Copy(TObject &) methods in hist classes #10942

Properly use Copy(TObject &) methods in hist classes #10942

Conversation

linev commented Jul 11, 2022

phsft-bot commented Jul 11, 2022

phsft-bot commented Jul 11, 2022

Failing tests:

phsft-bot commented Jul 11, 2022

Failing tests:

phsft-bot commented Jul 11, 2022

Failing tests:

phsft-bot commented Jul 11, 2022

Failing tests:

phsft-bot commented Jul 11, 2022

Failing tests:

phsft-bot commented Jul 11, 2022

Failing tests:

phsft-bot commented Jul 11, 2022

Failing tests:

lmoneta left a comment • edited Loading

Choose a reason for hiding this comment

linev commented Aug 3, 2022

lmoneta commented Aug 3, 2022

linev commented Aug 3, 2022

lmoneta commented Aug 3, 2022 • edited Loading

linev commented Aug 3, 2022

phsft-bot commented Aug 17, 2022

phsft-bot commented Aug 17, 2022

Failing tests:

phsft-bot commented Aug 17, 2022

Failing tests:

phsft-bot commented Aug 17, 2022

Failing tests:

phsft-bot commented Aug 17, 2022

Failing tests:

phsft-bot commented Aug 17, 2022

Failing tests:

phsft-bot commented Aug 17, 2022

Failing tests:

lmoneta left a comment

Choose a reason for hiding this comment

linev commented Aug 17, 2022

phsft-bot commented Aug 17, 2022

Errors:

Properly use `Copy(TObject &)` methods in `hist` classes #10942

Properly use `Copy(TObject &)` methods in `hist` classes #10942

lmoneta left a comment •

edited

Loading

lmoneta commented Aug 3, 2022 •

edited

Loading