Skip to content

Feature/k shape#80

Merged
avilchess merged 15 commits intomasterfrom
feature/k-shape
Feb 18, 2019
Merged

Feature/k shape#80
avilchess merged 15 commits intomasterfrom
feature/k-shape

Conversation

@avilchess
Copy link
Copy Markdown
Contributor

@avilchess avilchess commented Feb 15, 2019

Make sure you have checked all steps below.

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Benchmarks

  • My PR adds the following micro benchmarks OR does not need benchmarks for this extremely good reason:

Commits

  • My commits have been squashed if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

License

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 15, 2019

Codecov Report

Merging #80 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
+ Coverage   98.61%   98.68%   +0.07%     
==========================================
  Files          13       13              
  Lines        1511     1603      +92     
==========================================
+ Hits         1490     1582      +92     
  Misses         21       21
Impacted Files Coverage Δ
src/khiva/clustering.cpp 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1536190...03e186e. Read the comment docs.

Comment thread bindings/c/include/khiva_c/clustering.h Outdated
* @param maxIterations The maximum number of iterations allowed.
*/
KHIVAAPI void kMeans(khiva_array *tss, int *k, khiva_array *centroids, khiva_array *labels, float *tolerance,
int *maxIterations);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use snake case in this var name

Comment thread bindings/c/include/khiva_c/clustering.h Outdated
* @param centroids The resulting means or centroids.
* @param labels The resulting labels of each time series which is the closest centroid.
* @param tolerance The error tolerance to stop the computation of the centroids.
* @param maxIterations The maximum number of iterations allowed.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

don't forget to change it here too

Comment thread bindings/c/include/khiva_c/clustering.h Outdated
* @param centroids The resulting means or centroids.
* @param labels The resulting labels of each time series which is the closest centroid.
* @param tolerance The error tolerance to stop the computation of the centroids.
* @param maxIterations The maximum number of iterations allowed.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

Comment thread bindings/c/include/khiva_c/clustering.h Outdated
* @param maxIterations The maximum number of iterations allowed.
*/
KHIVAAPI void kShape(khiva_array *tss, int *k, khiva_array *centroids, khiva_array *labels, float *tolerance,
int *maxIterations);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

Comment thread bindings/c/src/clustering.cpp Outdated
#include <khiva_c/clustering.h>

void kMeans(khiva_array *tss, int *k, khiva_array *centroids, khiva_array *labels, float *tolerance,
int *maxIterations) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

Comment thread bindings/c/src/clustering.cpp Outdated
}

void kShape(khiva_array *tss, int *k, khiva_array *centroids, khiva_array *labels, float *tolerance,
int *maxIterations) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

*/
JNIEXPORT jlongArray JNICALL Java_io_shapelets_khiva_Clustering_kMeans(JNIEnv *env, jobject, jlong ref_tss, jint k,
jlong ref_centroids, jlong ref_labels,
jfloat tolerance, jint maxIterations);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

here it is ok, don't change it

Comment thread src/khiva/clustering.cpp Outdated
// Extract first EigenVector
af::array first = af::lookup(evectors, indMaxEigenValue, 1);

// TODO: Check this normalization is necessary.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Check if this

Comment thread src/khiva/clustering.cpp Outdated
float error = std::numeric_limits<float>::max();
int iter = 0;

// Stop Criteria: Stop updating after convergence is reached.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

... or the max number of iterations is reached.

Comment thread test/clusteringTest.cpp Outdated

unsigned int idxh[5] = {0, 1, 2, 0, 1};

float expected_c[35] = {-0.5234, 0.1560, -0.3627, -1.2764, -0.7781, 0.9135, 1.8711,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use CamelCase in this file. Applies to other variables as well

Copy link
Copy Markdown
Contributor

@otorreno otorreno left a comment

Choose a reason for hiding this comment

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

Take a look at the comments and merge eigenValues and eigenVectors functions

Copy link
Copy Markdown
Contributor

@otorreno otorreno left a comment

Choose a reason for hiding this comment

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

LGTM

@avilchess avilchess merged commit de72e85 into master Feb 18, 2019
@avilchess avilchess deleted the feature/k-shape branch February 18, 2019 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants