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

replaced SGVector::scale() with linalg::scale() #2928

Closed
wants to merge 8 commits into from
Closed

replaced SGVector::scale() with linalg::scale() #2928

wants to merge 8 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 31, 2015

It's the first part of the issue #2717

@@ -35,7 +38,7 @@ SGVector<float64_t> CMeanRule::combine(const SGMatrix<float64_t>& ensemble_resul
SGVector<float64_t> mean_labels(row_sum, ensemble_result.num_rows);

float64_t scale = 1/(float64_t)ensemble_result.num_cols;
mean_labels.scale(scale);
linalg::scale<linalg::Backend::NATIVE>(mean_labels, scale);
Copy link
Member

Choose a reason for hiding this comment

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

you have already written using namespace linalg, so you can omit the linalg:: here I suppose.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed. linalg:: was necessary to avoid names conflist with float64_t scale.

@lambday
Copy link
Member

lambday commented Nov 3, 2015

Hi @nginn. Thanks for the patch! It looks good. Please see my minor comments. We can merge it once you address those.

@karlnapf
Copy link
Member

updates here?

template<class T>
SGVector<T> SGVector<T>::operator+= (SGVector<T> x)
{
linalg::add<linalg::Backend::NATIVE>(*this, x, *this);
Copy link
Member

Choose a reason for hiding this comment

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

what is the a reason for NATIVE here?

Copy link
Author

Choose a reason for hiding this comment

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

@karlnapf I've did that according to @lambday,

So, for now you should just use NATIVE backend explicitly for the task.

Do I miss something in this situation?

@karlnapf
Copy link
Member

can you remove the NATIVE, and just use the default backend (is set to eigen3 these days)

@@ -25,6 +25,7 @@
#include <algorithm>

#include <shogun/mathematics/eigen3.h>
#include <shogun/mathematics/linalg/linalg.h>
Copy link
Member

Choose a reason for hiding this comment

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

needs to be guarded

@karlnapf karlnapf closed this Dec 17, 2017
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.

None yet

2 participants