TMVA MultiProcessing #858

mammadhajili · 2017-08-15T13:49:56Z

Mulltiprocessing of Hyper Parameter Optimisation, Cross Validation and Variable Importance

phsft-bot · 2017-08-15T13:49:58Z

Can one of the admins verify this patch?

lmoneta · 2017-08-15T13:50:44Z

phsft-bot · 2017-08-15T13:50:59Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

lmoneta · 2017-08-16T13:36:42Z

Hi Mammad,

Can you please update your Pull request with the fixes for the formatting (clang-format) and also
ass a tutorial program showing the usage of these 3 features in parallel.

Thank you

Lorenzo

martinmine · 2017-08-18T09:35:31Z

@phsft-bot build

phsft-bot · 2017-08-18T09:35:51Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

ashlaban

Hi, Thanks for your hard work Mammad! I include some general comments on how to improve the code and better integrate it into TMVA.

I didn't go through every line, so some comments might be applicable to more places than I indicated :)

ashlaban · 2017-08-23T13:35:52Z

tmva/tmva/src/HyperParameterOptimisation.cxx

    };
    vector<map<TString,Double_t>> res;
    auto nWorkers = TMVA::gConfig().NWorkers();
+    cout << "Number of Workers : " << TMVA::gConfig().NWorkers() << endl;


These seem like debug print outs, please use Log() << kDEBUG << ... << Endl; for these kind of messages.

ashlaban · 2017-08-23T13:38:17Z

tmva/tmva/src/HyperParameterOptimisation.cxx

    fFomType("Separation"),
    fFitType("Minuit"),
-    fNumFolds(5),
+    fNumFolds(4),


Prefer to change as little as possible (makes it easier to merge with other people's work!). I'd keep this as the old value unless there is a good motivation for this change. (This principle applies to more places)

ashlaban · 2017-08-23T13:39:10Z

tmva/tmva/inc/LinkDef4.h

 #pragma link C++ function TMVA::CreateVariableTransform;
 #pragma link C++ function TMVA::DataLoaderCopy;

+#pragma link C++ function TMVA::DataLoaderCopy;


Should we also export the DataLoaderMP copy here?

ashlaban · 2017-08-23T14:41:16Z

tmva/tmva/src/CrossValidation.cxx

+      res = workers.Map(workItem, ROOT::TSeqI(fNumFolds));
+  }
+
+  else {


Use the structure

if () { ... } else { ... }

ashlaban · 2017-08-23T14:41:23Z

tmva/tmva/src/DataLoader.cxx

+		  des->AddBackgroundTree((*treeinfo).GetTree(), (*treeinfo).GetWeight(),(*treeinfo).GetTreeType());
   }
 }
+std::vector<std::shared_ptr<TFile>> TMVA::DataLoaderCopyMP(TMVA::DataLoader *des, TMVA::DataLoader *src)


Add some documentation on what the method does, information about assumptions if there are any etc.

ashlaban · 2017-08-23T14:41:58Z

tmva/tmva/src/DataLoader.cxx

+    des->AddBackgroundTree(backgTree);
+
+    vec_files.push_back(sfile);
+    vec_files.push_back(bfile);


Can these commented out lines be removed?

ashlaban · 2017-08-23T14:48:14Z

tmva/tmva/inc/TMVA/DataLoader.h

      ClassDef(DataLoader,3);
   };
   void DataLoaderCopy(TMVA::DataLoader* des, TMVA::DataLoader* src);
+   std::vector<std::shared_ptr<TFile>> DataLoaderCopyMP(TMVA::DataLoader *des, TMVA::DataLoader *src);


Do these need to be public? Would it be better to use private with Envelope as a friend? I think the intention is that only the Envelope should be able to use it. I would then propose that we enforce this. We can always open the interface up later, but not the other way around.

@mammadhajili could you answer the comments of @ashlaban please?

ashlaban · 2017-08-23T15:57:06Z

@phsft-bot build!

phsft-bot · 2017-08-23T15:57:15Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

ashlaban · 2017-08-23T17:43:22Z

@phsft-bot build!

phsft-bot · 2017-08-23T17:43:30Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

martinmine · 2017-08-24T08:24:41Z

@phsft-bot build!

martinmine · 2017-08-24T08:27:33Z

You might want to squash or fixup your clang-format commits.

martinmine · 2017-08-24T08:33:23Z

@phsft-bot build

phsft-bot · 2017-08-24T08:33:32Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

phsft-bot · 2017-08-24T09:16:25Z

Build failed on ubuntu14/native.
See console output.

Failing tests:

projectroot.roottest.python.JupyROOT.roottest_python_JupyROOT_simpleCppMagic

martinmine · 2017-08-24T09:32:40Z

@phsft-bot build

phsft-bot · 2017-08-24T09:32:51Z

Starting build on centos7/gcc49, mac1012/native, slc6/gcc49, slc6/gcc62, ubuntu14/native with flags -Dvc=OFF -Dimt=ON -Dccache=ON
How to customize builds

bellenot · 2017-11-28T08:21:29Z

@lmoneta can you take care of this (or close it)

etejedor · 2018-02-20T13:08:59Z

@lmoneta @ashlaban all checks pass but there are some comments to be addressed, could you make sure this is followed or close the PR if not needed anymore?

ashlaban · 2018-03-19T12:04:07Z

Sorry for the late response. Yes, we'll make sure to follow up on this PR.

etejedor · 2018-04-26T08:30:35Z

@lmoneta @ashlaban any progress on merging this PR since our last chat?

etejedor · 2018-07-26T11:02:38Z

@lmoneta @ashlaban any progress on merging this PR since our last chat?

ashlaban · 2018-07-26T11:13:36Z

Cross validation has been integrated through a different PR. Remaining are the Hyper param opt and variable importance.
I refer to @lmoneta for the future plan there.

It is my understanding however that if we integrate them we do it through new PR’s, indicating that this PR can be closed.

I will make sure to update the PR after discussion with Lorenzo during next week.

ashlaban · 2018-08-07T09:46:52Z

These changes will be implemented in separate PR's.

mammadhajili force-pushed the TMVA-Parallel branch from 150aaf6 to bc679f6 Compare August 16, 2017 13:58

mammadhajili and others added 3 commits August 18, 2017 11:31

TMVA MultiProcessing

b191f1b

Fixed clang-format issues

6b66171

Loading files fixes and clang format

691f6cd

mammadhajili force-pushed the TMVA-Parallel branch from bc679f6 to 691f6cd Compare August 18, 2017 09:33

ashlaban reviewed Aug 23, 2017

View reviewed changes

mammadhajili requested a review from Axel-Naumann as a code owner August 23, 2017 15:51

Fixing clang-format issues

d34f639

mammadhajili force-pushed the TMVA-Parallel branch from 8ae1c1e to d34f639 Compare August 24, 2017 08:33

xvallspl assigned lmoneta Sep 25, 2017

ashlaban mentioned this pull request Apr 17, 2018

[TMVA] Add multiprocessing to parallelise CV #1869

Merged

ashlaban closed this Aug 7, 2018

phsft-bot mentioned this pull request Apr 20, 2022

[RF] Remove deprecated version of createHistogram to see what happens #10419

Closed

phsft-bot mentioned this pull request Jun 27, 2023

[RF] Add methods to create owning RooFit proxies via std::unique_ptr #12924

Merged

TMVA MultiProcessing #858

TMVA MultiProcessing #858

Uh oh!

Conversation

mammadhajili commented Aug 15, 2017

Uh oh!

phsft-bot commented Aug 15, 2017

Uh oh!

lmoneta commented Aug 15, 2017

Uh oh!

phsft-bot commented Aug 15, 2017

Uh oh!

lmoneta commented Aug 16, 2017

Uh oh!

martinmine commented Aug 18, 2017

Uh oh!

phsft-bot commented Aug 18, 2017

Uh oh!

ashlaban left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ashlaban commented Aug 23, 2017

Uh oh!

phsft-bot commented Aug 23, 2017

Uh oh!

ashlaban commented Aug 23, 2017

Uh oh!

phsft-bot commented Aug 23, 2017

Uh oh!

martinmine commented Aug 24, 2017

Uh oh!

martinmine commented Aug 24, 2017

Uh oh!

martinmine commented Aug 24, 2017

Uh oh!

phsft-bot commented Aug 24, 2017

Uh oh!

phsft-bot commented Aug 24, 2017

Failing tests:

Uh oh!

martinmine commented Aug 24, 2017

Uh oh!

phsft-bot commented Aug 24, 2017

Uh oh!

bellenot commented Nov 28, 2017

Uh oh!

etejedor commented Feb 20, 2018

Uh oh!

ashlaban commented Mar 19, 2018

Uh oh!

etejedor commented Apr 26, 2018

Uh oh!

etejedor commented Jul 26, 2018

Uh oh!

ashlaban commented Jul 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashlaban commented Aug 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ashlaban commented Jul 26, 2018 •

edited

Loading