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

[TMVA] Add operations needed for performing optimizer updates: #2315

Merged
merged 1 commit into from Jul 23, 2018

Conversation

6 participants
@ravikiran0606
Copy link
Contributor

commented Jul 12, 2018

  • Implement ConstAdd, ConstMult, ReciprocalElementWise, SquareElementWise, SqrtElementWise in CPU, GPU and Reference architectures.

  • Add Unit Tests for them in CPU, GPU architecture.

  • Add ROOT Style docs and clang format the code.

@ravikiran0606 ravikiran0606 requested a review from lmoneta as a code owner Jul 12, 2018

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 12, 2018

Can one of the admins verify this patch?

@Axel-Naumann

This comment has been minimized.

Copy link
Member

commented Jul 13, 2018

Please rebase onto master (rewriting this branch's history) instead of creating merge commits.

@ravikiran0606

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2018

@Axel-Naumann Yeah I was trying to do that! I'll rebase into master and update this PR.

@ravikiran0606 ravikiran0606 force-pushed the ravikiran0606:Update-Operations branch from f92616c to 3668dcf Jul 15, 2018

@ravikiran0606 ravikiran0606 force-pushed the ravikiran0606:Update-Operations branch from 3668dcf to dc6f823 Jul 15, 2018

@ravikiran0606

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2018

I have rebased into master and updated the PR. @lmoneta @stwunsch Can you please review this?

@stwunsch

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2018

@phsft-bot build

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2018

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

@ravikiran0606 ravikiran0606 force-pushed the ravikiran0606:Update-Operations branch from dc6f823 to 5dc0059 Jul 15, 2018

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 15, 2018

Build failed on centos7/clang39.
See console output.

Errors:

  • ERROR: Timeout after 30 minutes
  • ERROR: Error fetching remote repo 'origin'
  • ERROR: Error fetching remote repo 'origin'
  • ERROR: Timeout after 30 minutes
  • ERROR: Error fetching remote repo 'origin'
  • ERROR: Error fetching remote repo 'origin'
@xvallspl

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

@phsft-bot build!

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 16, 2018

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

@ravikiran0606 ravikiran0606 force-pushed the ravikiran0606:Update-Operations branch from 5dc0059 to d888dad Jul 16, 2018

@ravikiran0606

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2018

@stwunsch Can you please trigger the build?

@lmoneta

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

@phsft-bot build!

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 17, 2018

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

@lmoneta
Copy link
Contributor

left a comment

Hi,
Do we really need these functions to be implemented for all architectures ?
In this case we miss the GPU implementation.
I think since these functions affect the optimiser, maybe they could be implemented for only one type of matrices, the reference one. To be discussed

@lmoneta

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

Looking carefully at the code, I see that we need to have a GPU implementations for these functions.
As a first attempt we could try by doing in term of TMatrixT using the conversions TCudaMatrix -> Matrix functions and then we check if it is really needed to have something specific for GPU's

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 17, 2018

Build failed on slc6/gcc48.
See console output.

Errors:

  • ERROR: Timeout after 30 minutes
  • ERROR: Error cloning remote repo 'origin'
  • ERROR: Error cloning remote repo 'origin'
@lmoneta

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2018

@phsft-bot build!

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2018

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

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2018

Build failed on slc6-i686/gcc49.
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'
@ravikiran0606

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

@lmoneta @stwunsch I ll add the GPU implementation for those Operations soon and update this PR. We can merge then !

[TMVA] Add operations needed for performing optimizer updates:
* Implement ConstAdd, ConstMult, ReciprocalElementWise, SquareElementWise, SqrtElementWise in CPU, GPU and Reference architectures.

* Add Unit Tests for them in CPU, GPU architecture.

* Add ROOT Style docs and clang format the code.

@ravikiran0606 ravikiran0606 force-pushed the ravikiran0606:Update-Operations branch from d888dad to 27d6da7 Jul 19, 2018

@ravikiran0606

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

@stwunsch @lmoneta I added the GPU implementation for those operations and both the CPU and GPU tests are passing. Can you review and trigger the build?

The update operations tests are as follows,

update operations

update operations tests

@stwunsch

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2018

@phsft-bot build

@phsft-bot

This comment has been minimized.

Copy link
Collaborator

commented Jul 20, 2018

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

@stwunsch stwunsch merged commit a85bdf0 into root-project:master Jul 23, 2018

2 checks passed

Jenkins CI build Build passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ravikiran0606 ravikiran0606 deleted the ravikiran0606:Update-Operations branch Jul 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.