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

make guassian get rid of LAPACK #3819

Merged
merged 1 commit into from May 24, 2017

Conversation

MikeLing
Copy link
Contributor

Those function names like 'cblas_dger' doesn't make sense to me.

@@ -53,4 +53,4 @@ if [[ $# -ne 2 ]]; then
fi

# Run the check
check_shogun_style ${1:-} ${2:-}
check_shogun_style ${1:-} ${2:-}
Copy link
Member

Choose a reason for hiding this comment

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

do we need this?

CblasRowMajor, num_dim, num_dim, alpha_k[j], v.vector, 1,
v.vector, 1, (double*)cov_sum.matrix, num_dim);
#else
linalg::cblas_dger<float64_t>(alpha_k[j], v, v, cov_sum);
Copy link
Member

Choose a reason for hiding this comment

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

please use a different name, dont use cblas_dger

cblas_dgemv(CblasRowMajor, CblasNoTrans, m_d.vlen, m_d.vlen,
1, m_u.matrix, m_d.vlen, difference, 1, 0, temp_holder, 1);
#else
linalg::cblas_dgemv<float64_t>(
Copy link
Member

Choose a reason for hiding this comment

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

same here.... use a better name for the function cblas_dgemv

@MikeLing MikeLing force-pushed the get_rid_of_LAPACK branch 5 times, most recently from 833e82a to 4a894aa Compare May 21, 2017 07:20
* @param vector y
*/
template <typename T>
void cblas_dgemv(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vigsterkr how about vvpm which means vector produce vector then plus matrix . I know it still looks weird, but I can't find a better name to represent this function does like vector produce vector then plus matrix

Copy link
Member

Choose a reason for hiding this comment

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

ok so let's just have it without the cblas_ prefix as @lisitsyn suggested... so dgemv

@MikeLing MikeLing force-pushed the get_rid_of_LAPACK branch 2 times, most recently from 0b2fec2 to c4007ea Compare May 24, 2017 02:00
@MikeLing
Copy link
Contributor Author

Ask review for this pr. I guess it's ready to reviewed except the function names :)

@vigsterkr Do you have time to take a look at this? Thank you!

@@ -350,6 +350,26 @@ Container<T> add(Container<T>& a, Container<T>& b, T alpha=1, T beta=1)
}

/**
* Performs the operation A = alpha * x * y' + A
*
* @param scaling factor for vector x
Copy link
Member

Choose a reason for hiding this comment

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

you mean:

@param alpha scaling factor for vector x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, thank you.

* and multiplies the resulting matrix by alpha. It then multiplies vector y by
* beta. It stores the sum of these two products in vector y
*
* @param scaling factor for vector ax
Copy link
Member

Choose a reason for hiding this comment

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

first word after the @param should be always the name of the variable in the function description

* beta. It stores the sum of these two products in vector y
*
* @param scaling factor for vector ax
* @param vector a
Copy link
Member

Choose a reason for hiding this comment

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

isn't this a matrix? (2nd parameter of the function)

* @param scaling factor for vector ax
* @param vector a
* @param Whether to transpose matrix a
* @param vector x
Copy link
Member

Choose a reason for hiding this comment

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

this should be

@param x vector

* It then multiplies matrix c by beta. It stores the sum of these two products
* in matrix c.
*
* @param scaling factor for matrix a*b
Copy link
Member

Choose a reason for hiding this comment

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

same here as above... the doxygen needs to be fixed

@vigsterkr
Copy link
Member

@MikeLing cool let's see what the CI does.

@vigsterkr vigsterkr merged commit 380c06f into shogun-toolbox:develop May 24, 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