Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

speedup for .class.C for MLP #121

Merged
merged 25 commits into from
Aug 2, 2018
Merged

Conversation

pseyfert
Copy link
Contributor

I ported the changes from pseyfert/tmva-mlp to the code generation. As a test I just ran the class.C network resulting from tutorials/tmva/TMVAClassification.C (with "MLP") and evaluated it similar to tutorials/tmva/TMVAClassificationApplication.C. According to callgrind the network evaluation is ~17% cpu cycles faster.
NB: i did not port all changes from pseyfert/tmva-mlp - I did not import the SSE/AVX intrinsics and avoided what seemed too difficult (optimising the putIndices and getIndices out)

@pseyfert
Copy link
Contributor Author

just added ec7918b (that switch hardly seemed to do do anything)

@peremato peremato assigned lmoneta and unassigned lmoneta Mar 1, 2017
@pseyfert
Copy link
Contributor Author

pseyfert commented Mar 9, 2017

looks like the conflict is purely whitespace changes on master, mergeing by hand.

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@pseyfert
Copy link
Contributor Author

this is not superseded by #572 (though merging both might result in conflicts. i can work that out in #121 once #572 is merged).

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@vgvassilev
Copy link
Member

Could you use git rebase master on this branch instead. We don't like these merge commits...

@pseyfert
Copy link
Contributor Author

I think that's what I meant. My idea was:

@pseyfert pseyfert force-pushed the tmva-mlp-codegen branch from 9ccce9e to ef160e4 Compare May 26, 2017 18:54
@pseyfert
Copy link
Contributor Author

just rebased (harder than expected). Let me double check I didn't mess up.

@vgvassilev
Copy link
Member

@phsft-bot build!

@pseyfert, I was wondering can we create a gtest testing that part? This is good for test coverage and documentation purposes...

@phsft-bot
Copy link
Collaborator

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

@phsft-bot
Copy link
Collaborator

Build failed on mac1012/native.
See console output.

Errors:

  • /Volumes/HDD2/ec/build/workspace/root-pullrequests-build/root/tmva/tmva/src/MethodANNBase.cxx:1120:74: error: expected ';' after expression

@phsft-bot
Copy link
Collaborator

Build failed on ubuntu14/native.
See console output.

Errors:

  • /mnt/vdb/lsf/workspace/root-pullrequests-build/root/tmva/tmva/src/MethodANNBase.cxx:1121:7: error: expected ‘;’ before ‘}’ token

@phsft-bot
Copy link
Collaborator

Build failed on centos7/gcc49.
See console output.

Errors:

  • /mnt/build/jenkins/workspace/root-pullrequests-build/root/tmva/tmva/src/MethodANNBase.cxx:1121:7: error: expected ‘;’ before ‘}’ token

@phsft-bot
Copy link
Collaborator

Build failed on slc6/gcc62.
See console output.

Errors:

  • /mnt/build/jenkins/workspace/root-pullrequests-build/root/tmva/tmva/src/MethodANNBase.cxx:1121:7: error: expected ‘;’ before ‘}’ token

@phsft-bot
Copy link
Collaborator

Build failed on slc6/gcc49.
See console output.

Errors:

  • /mnt/build/jenkins/workspace/root-pullrequests-build/root/tmva/tmva/src/MethodANNBase.cxx:1121:7: error: expected ‘;’ before ‘}’ token

@vgvassilev
Copy link
Member

@pseyfert ping...

@pseyfert
Copy link
Contributor Author

pseyfert commented Jul 4, 2017

back on this one.

it appears my change is not super numerically stable. I ran the network from tutorials/tmva/TMVAClassification and the floating point comparison from https://stackoverflow.com/a/253874 with rapidcheck and this check

    if (!( essentiallyEqual(reference , newimplementation , 1000.f*std::numeric_limits<float>::epsilon()) )) {
      RC_ASSERT( refval == safval) ;
    }

and get a failure every ~ 500 tests (i.e. at that rate the numerical noise i generate by changing the network implementation exceeds more than 1000 times floating point precision (NB: the return value is actually a double …))

but this only if i disable the fast tanh evaluation (enabled by default). If i enable the fast tanh evaluation it appears response values change by more than that.
[background: the TMVA::Reader already uses the fast evaluation, the codegen wasn't. so this should actually improve actually the consistency between TMVA::Reader usage and the codegen - i was only testing old codegen vs new codegen]

will discuss at the next tmva meeting, with a bit more insight.

for the build failure, sorry that was a pretty stupid merging mistake i should've seen (… if i had compiled it locally before pushing … instead of postponing that to running the numeric test).

@vgvassilev
Copy link
Member

ping.

@pseyfert
Copy link
Contributor Author

found the issue. ran numeric test here https://github.com/pseyfert/tmva-codegen-rapidcheck

@vgvassilev
Copy link
Member

Great! Could you add a gtest incorporating the tmva-codegen-rapidcheck? You can see how it's done here.

@vgvassilev
Copy link
Member

@phsft-bot build!

@phsft-bot
Copy link
Collaborator

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
Copy link
Collaborator

Build failed on slc6/gcc49.
See console output.

Errors:

  • ERROR: Timeout after 10 minutes
  • ERROR: Error fetching remote repo 'origin'
  • stderr: error: RPC failed; result=18, HTTP code = 200
  • ERROR: Error fetching remote repo 'origin'

@ashlaban
Copy link
Contributor

ashlaban commented Sep 8, 2017

@phsft-bot build!

@phsft-bot
Copy link
Collaborator

Build failed on slc6/gcc62.
See console output.

Errors:

  • ERROR: Timeout after 30 minutes
  • ERROR: Error fetching remote repo 'origin'
  • stderr: error: RPC failed; result=18, HTTP code = 200
  • ERROR: Error fetching remote repo 'origin'

@phsft-bot
Copy link
Collaborator

Build failed on slc6/gcc62.
See console output.

Errors:

  • ERROR: Timeout after 30 minutes
  • ERROR: Error fetching remote repo 'origin'
  • stderr: error: RPC failed; result=18, HTTP code = 200
  • ERROR: Error fetching remote repo 'origin'

@etejedor
Copy link
Contributor

@ashlaban @lmoneta what is the status of this PR? Should we try again to build and merge?

@etejedor
Copy link
Contributor

@phsft-bot build

@ashlaban
Copy link
Contributor

LGTM. I think Lorenzo will be back from vacation on the 28th.

@phsft-bot
Copy link
Collaborator

Starting build on slc6/gcc48, slc6-i686/gcc49, centos7/clang39, centos7/gcc62, centos7/gcc7, fedora28/native, ubuntu16/native, mac1013/native, windows10/vc15 with flags -Dimt=ON -Dccache=ON
How to customize builds

@etejedor
Copy link
Contributor

Great @ashlaban , can you ping @lmoneta when he is back about this? This is a senior PR, opened in 2015! :)

@ashlaban
Copy link
Contributor

@phsft-bot build!

@phsft-bot
Copy link
Collaborator

Starting build on slc6/gcc48, slc6-i686/gcc49, centos7/clang39, centos7/gcc62, centos7/gcc7, fedora28/native, ubuntu16/native, mac1013/native, windows10/vc15 with flags -Dimt=ON -Dccache=ON
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on ubuntu16/native.
See console output.

Errors:

  • Error: could not find CMAKE_PROJECT_NAME in Cache

@phsft-bot
Copy link
Collaborator

Build failed on fedora28/native.
See console output.

Errors:

  • Error: could not find CMAKE_PROJECT_NAME in Cache

@phsft-bot
Copy link
Collaborator

Build failed on windows10/vc15.
See console output.

@phsft-bot
Copy link
Collaborator

Build failed on mac1013/native.
See console output.

Errors:

  • Error: could not find CMAKE_PROJECT_NAME in Cache

@phsft-bot
Copy link
Collaborator

Build failed on slc6-i686/gcc49.
See console output.

Errors:

  • Error: could not find CMAKE_PROJECT_NAME in Cache

@Axel-Naumann
Copy link
Member

@phsft-bot build!

@phsft-bot
Copy link
Collaborator

Starting build on slc6/gcc48, slc6-i686/gcc49, centos7/clang39, centos7/gcc62, centos7/gcc7, fedora28/native, ubuntu16/native, mac1013/native, windows10/vc15 with flags -Dimt=ON -Dccache=ON
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on ubuntu16/native.
See console output.

@Axel-Naumann
Copy link
Member

@phsft-bot build just on ubuntu16/native

@phsft-bot
Copy link
Collaborator

Starting build on ubuntu16/native with flags -Dimt=ON -Dccache=ON
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on ubuntu16/native.
See console output.

@Axel-Naumann
Copy link
Member

@phsft-bot build just on ubuntu16/native

@phsft-bot
Copy link
Collaborator

Starting build on ubuntu16/native with flags -Dimt=ON -Dccache=ON
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on ubuntu16/native.
See console output.

@Axel-Naumann
Copy link
Member

@phsft-bot build just on ubuntu16/native

@phsft-bot
Copy link
Collaborator

Starting build on ubuntu16/native with flags -Dimt=ON -Dccache=ON
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on ubuntu16/native.
See console output.

@Axel-Naumann
Copy link
Member

@phsft-bot build just on ubuntu16/native

@phsft-bot
Copy link
Collaborator

Starting build on ubuntu16/native with flags -Dimt=ON -Dccache=ON
How to customize builds

@lmoneta lmoneta merged commit b7a3854 into root-project:master Aug 2, 2018
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.

9 participants