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.

Sign up for GitHub

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

Jump to bottom

linalg sum method #3278

Closed
wants to merge 2 commits into from
Closed

linalg sum method #3278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
56124cf
sum method
OXPHOS Jun 10, 2016
54f0f91
update
OXPHOS Jun 10, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/shogun/mathematics/linalg/CPUBackend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,20 @@ T CPUBackend::dot(const CPUVector<T> &a, const CPUVector<T> &b) const
return vec_a.dot(vec_b);
}

template <typename T>
T CPUBackend::sum(const CPUVector<T> &vec) const
{
typedef Eigen::Matrix<T, Eigen::Dynamic, 1> VectorXt;
Eigen::Map<VectorXt> v(vec.CPUptr, vec.vlen);;
return v.sum();
}

template int32_t CPUBackend::dot<int32_t>(const CPUVector<int32_t> &a, const CPUVector<int32_t> &b) const;
template float32_t CPUBackend::dot<float32_t>(const CPUVector<float32_t> &a, const CPUVector<float32_t> &b) const;

template int32_t CPUBackend::sum<int32_t>(const CPUVector<int32_t> &vec) const;
template float32_t CPUBackend::sum<float32_t>(const CPUVector<float32_t> &vec) const;

}

#endif //HAVE_CXX11
9 changes: 9 additions & 0 deletions src/shogun/mathematics/linalg/CPUBackend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,15 @@ class CPUBackend
*/
template <typename T>
T dot(const CPUVector<T> &a, const CPUVector<T> &b) const;

/**
* Method that computes the sum of SGVectors using Eigen3
Copy link
Member

Choose a reason for hiding this comment

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

I would remove "with Eigen3" here

*
* @param vec a vector whose sum has to be computed
* @return the vector sum \f$\sum_i a_i\f$
*/
template <typename T>
T sum(const CPUVector<T> &vec) const;
};

}
Expand Down
15 changes: 15 additions & 0 deletions src/shogun/mathematics/linalg/GPUBackend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#ifdef HAVE_VIENNACL
#include <viennacl/vector.hpp>
#include <viennacl/linalg/inner_prod.hpp>
#include <viennacl/linalg/sum.hpp>
#endif
Copy link
Member

Choose a reason for hiding this comment

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

why close here the #ifdef HAVE_VIENNACL ?
i mean if you don't have viennacl this whole implementation is just foobar.
do it like

#ifdef HAVE_VIENNACL

... the whole GPUBackend implementation...
#endif

Copy link
Member

Choose a reason for hiding this comment

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

Agreed! The creation of the class itself should fail if viennacl is not found.


#ifdef HAVE_CXX11
Expand All @@ -60,9 +61,23 @@ T GPUBackend::dot(const GPUVector<T> &a, const GPUVector<T> &b) const
#endif
}

template <typename T>
T GPUBackend::sum(const GPUVector<T> &vec) const
{
#ifdef HAVE_VIENNACL
return viennacl::linalg::sum(vec.gpuarray->GPUvec());
#else
SG_SERROR("User did not register GPU backend. \n");
Copy link
Member

Choose a reason for hiding this comment

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

I don't like it if we would have to put this error message in every implementation.

Minor: No space after period.

Copy link
Member

Choose a reason for hiding this comment

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

why do we generate at all GPUBackend class if there's no gpu backend? i'm sorry but i'm very very confused what's happening here...

you should implement a GPUBackend using for example viannacl IF it's available. if not you just don't create one. like many other models, that if LAPACK was not present, the implementation was not built.

return T(0);
#endif
}

template int32_t GPUBackend::dot<int32_t>(const GPUVector<int32_t> &a, const GPUVector<int32_t> &b) const;
template float32_t GPUBackend::dot<float32_t>(const GPUVector<float32_t> &a, const GPUVector<float32_t> &b) const;

template int32_t GPUBackend::sum<int32_t>(const GPUVector<int32_t> &vec) const;
template float32_t GPUBackend::sum<float32_t>(const GPUVector<float32_t> &vec) const;

}

#endif //HAVE_CXX11
9 changes: 9 additions & 0 deletions src/shogun/mathematics/linalg/GPUBackend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,15 @@ class GPUBackend
*/
template <typename T>
T dot(const GPUVector<T> &a, const GPUVector<T> &b) const;

/**
* Method that computes the sum of SGVectors using Eigen3
Copy link
Member

Choose a reason for hiding this comment

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

not Eigen3.
I suggest to remove the library from the doc. Also above
@lambday thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed!

*
* @param a vector whose sum has to be computed
* @return the vector sum \f$\sum_i a_i\f$
*/
template <typename T>
T sum(const GPUVector<T> &vec) const;
};

}
Expand Down
24 changes: 24 additions & 0 deletions src/shogun/mathematics/linalg/SGLinalg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,37 @@ T SGLinalg::dot(BaseVector<T> *a, BaseVector<T> *b) const
}
}

template <class T>
T SGLinalg::sum(BaseVector<T> *vec) const
{
if (vec->onGPU())
{
if (this->hasGPUBackend())
{
Copy link
Member

Choose a reason for hiding this comment

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

we dont use {} for single line statements

return m_gpubackend->sum<T>(static_cast<GPUVector<T>&>(*vec));
Copy link
Member

Choose a reason for hiding this comment

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

don't need to pass T in sum<T> I believe.

}
else
{
SG_SERROR("User did not register GPU backend. \n");
Copy link
Member

Choose a reason for hiding this comment

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

Why was there an error message in the sum implementation if there is already one here?

Also, can we just call some method that throws the error? We dont want to re-write this all the time.
Finally, can you please provide a better error message? Like "Tried to call GPU method %s without registering a GPU backend first. Register as blablabla"

return -1;
Copy link
Member

Choose a reason for hiding this comment

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

-1 ?

Copy link
Member

Choose a reason for hiding this comment

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

NaN in best case....

}
}
else
{
return m_cpubackend->sum<T>(static_cast<CPUVector<T>&>(*vec));
}
}

bool SGLinalg::hasGPUBackend() const
{
return m_gpubackend != nullptr;
}

template int32_t SGLinalg::dot<int32_t>(BaseVector<int32_t> *a, BaseVector<int32_t> *b) const;
template float32_t SGLinalg::dot<float32_t>(BaseVector<float32_t> *a, BaseVector<float32_t> *b) const;

template int32_t SGLinalg::sum<int32_t>(BaseVector<int32_t> *vec) const;
template float32_t SGLinalg::sum<float32_t>(BaseVector<float32_t> *vec) const;
}

#endif //HAVE_CXX11
9 changes: 9 additions & 0 deletions src/shogun/mathematics/linalg/SGLinalg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,15 @@ class SGLinalg
template <class T>
T dot(BaseVector<T> *a, BaseVector<T> *b) const;

/**
* Method that computes the sum of a vector
*
* @param vec a vector whose sum has to be computed
* @return the vector sum \f$\sum_i a_i\f$
*/
template <class T>
T sum(BaseVector<T> *vec) const;

/** Check whether gpubackend is registered by user */
bool hasGPUBackend() const;

Expand Down
30 changes: 30 additions & 0 deletions tests/unit/mathematics/SGLinalg_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,19 @@ TEST(SGLinalg, CPUBackend_dot)
EXPECT_NEAR(result, 20.0, 1E-15);
}

TEST(SGLinalg, CPUBackend_sum)
{
const index_t size=10;
Copy link
Member

Choose a reason for hiding this comment

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

This is not an optimal unit test.
Why don't you use range_fill, then at least we guard against a few more cases

SGVector<int32_t> a(size);
a.set_const(2);

CPUVector<int32_t> a_CPU(a);

auto result = sg_linalg->sum(&a_CPU);

EXPECT_NEAR(result, 20.0, 1E-15);
}

#ifdef HAVE_VIENNACL

TEST(SGLinalg, GPU_Vector_convert)
Expand Down Expand Up @@ -106,6 +119,23 @@ TEST(SGLinalg, GPUBackend_dot)
EXPECT_NEAR(result, 20.0, 1E-15);
}

TEST(SGLinalg, GPUBackend_sum)
{
const index_t size=10;
SGVector<int32_t> a(size);
a.set_const(2);

GPUVector<int32_t> a_GPU(a);
Copy link
Member

Choose a reason for hiding this comment

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

this guy now lives on CPU memory if no GPU is available?


std::shared_ptr<GPUBackend> ViennaCLBackend;
Copy link
Member

@karlnapf karlnapf Jun 10, 2016 •
edited

Choose a reason for hiding this comment

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

this needs to be guarded, no? UPDATE: I just saw it is, sorry

ViennaCLBackend = std::shared_ptr<GPUBackend>(new GPUBackend);

sg_linalg->set_gpu_backend(ViennaCLBackend);
Copy link
Member

Choose a reason for hiding this comment

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

I suggest you keep the GPU backend test in a different file, or at least move registering the backend to a method. To avoid having to register it for every test

auto result = sg_linalg->sum(&a_GPU);

EXPECT_NEAR(result, 20.0, 1E-15);
}

#endif //HAVE_VIENNACL

#endif //HAVE_CXX11