Skip to content

Conversation

@stwunsch
Copy link
Contributor

@stwunsch stwunsch commented May 6, 2020

No description provided.

@stwunsch stwunsch requested a review from oshadura as a code owner May 6, 2020 13:47
@stwunsch stwunsch self-assigned this May 6, 2020
@phsft-bot
Copy link

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos7-multicore/default, ROOT-fedora29/python3, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1015/cxx17, windows10/cxx14
How to customize builds

Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tutorial should not use automatically pymva in case it is not available.
I would not disable them.
The change should be in not calling
TMVA::PyMethodBase::PyInitialize()

Unfortunately I don't think we have a pre-processor macro for pymva. So we need something not really nice but like this:

int iret = gSystem->Load("libPyMVA");
 if (iret >=0) gROOT->ProcessLine("TMVA::PyMethodBase::PyInitialize()");

@phsft-bot
Copy link

Build failed on ROOT-performance-centos7-multicore/default.
Running on olhswep22.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See cdash.
See console output.

Errors:

  • [2020-05-06T14:07:01.995Z] 646/2022 Test [TDF] minor fixes #370: tutorial-dataframe-df103_NanoAODHiggsAnalysis .....................................................***Failed Error regular expression found in output. Regex=[: error:] 4.48 sec

Failing tests:

@stwunsch
Copy link
Contributor Author

stwunsch commented May 7, 2020

Wouldn't it be easier to just remove the pymva part? I think an explicit library load in a tutorial is a little bit weird, from the users perspective. It does not convey the message of a clean tutorial for a stable feature.

@lmoneta
Copy link
Member

lmoneta commented May 7, 2020

HI,
I think is nice to show in a tutorial that we can run different methods and TMVA and Keras at the same time.
We could add a pre-processing macro for pymva, something like R__HAS_PYMVA
or even better fix the loading of the library using the ROOT plugin-manager.

I will see if I can provide a PR for one of these tomorrow

@oshadura
Copy link
Collaborator

@stwunsch @lmoneta is this still something we would like to be merged?

@lmoneta
Copy link
Member

lmoneta commented May 27, 2020

I think we can close this PR. It is not needed anymore

@oshadura oshadura closed this May 29, 2020
@oshadura
Copy link
Collaborator

@stwunsch I am sorry I closed PR but please reopen it if you feel it is something still needed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants