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

Add mass and findBestNOccurrences to the public API #114

Merged
merged 1 commit into from
Jun 4, 2019

Conversation

jrecuerda
Copy link
Contributor

Some refactor has been performed in order to publish in the API the mass
function not requiring some intermediate computation as parameters.

The function findBestNOccurrences has been also added to the API. The C
and JNI bindings have been also included for these two functions

The corresponding unit tests have been created.

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.

Copy link
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.

Good refactor with the internal functions!

* @brief Calculates the Mueen distance.
*
* The result has the following structure:
* - 1st dimension contains the index of the subsequence in the tieme serie.
Copy link
Contributor

Choose a reason for hiding this comment

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

time

*
* The result has the following structure:
* - 1st dimension correspond the nth best match.
* - 2nd dimension conrrespond the numbef of queries.
Copy link
Contributor

Choose a reason for hiding this comment

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

corresponds to the number

* @brief Calculates the N best matches of several queries in several time series.
*
* The result has the following structure:
* - 1st dimension correspond the nth best match.
Copy link
Contributor

Choose a reason for hiding this comment

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

corresponds to

* - 2nd dimension conrrespond the numbef of queries.
* - 3rd dimension correspond the numbef of time series.
*
* For example, the distance in the position (1, 2, 3) correspond to the second best distance of the third query in the
Copy link
Contributor

Choose a reason for hiding this comment

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

corresponds (applies to more lines as well)

*
* The result has the following structure:
* - 1st dimension contains the index of the subsequence in the tieme serie.
* - 2nd dimension contains the numbef of queries.
Copy link
Contributor

Choose a reason for hiding this comment

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

number

* The result has the following structure:
* - 1st dimension contains the index of the subsequence in the tieme serie.
* - 2nd dimension contains the numbef of queries.
* - 3rd dimension contains the numbef of time series.
Copy link
Contributor

Choose a reason for hiding this comment

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

number

* - 3rd dimension contains the numbef of time series.
*
* For example, the distance in the position (1, 2, 3) correspond to the distance of the third query to the fourth time
* serie for the second subsequence in the time serie
Copy link
Contributor

Choose a reason for hiding this comment

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

series always ends with s

* @brief Calculates the moving average and standard deviation of the time series 't'.
* The result has the following structure:
* - 1st dimension correspond the nth best match.
* - 2nd dimension conrrespond the numbef of queries.
Copy link
Contributor

Choose a reason for hiding this comment

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

Apply the same comments, as this is copy pasted from the other, or viceversa

@jrecuerda jrecuerda force-pushed the feature/findBestNOccurrences branch from 6acdb0e to d7b4c4b Compare June 4, 2019 07:48
@codecov
Copy link

codecov bot commented Jun 4, 2019

Codecov Report

Merging #114 into master will decrease coverage by 0.07%.
The diff coverage is 98.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #114      +/-   ##
==========================================
- Coverage   97.75%   97.67%   -0.08%     
==========================================
  Files          16       16              
  Lines        1694     1722      +28     
==========================================
+ Hits         1656     1682      +26     
- Misses         38       40       +2
Impacted Files Coverage Δ
src/khiva/matrixInternal.cpp 100% <100%> (ø) ⬆️
src/khiva/matrix.cpp 98.87% <95.83%> (-0.81%) ⬇️

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 ceaabb4...a1f5daa. Read the comment docs.

* @param index Position where the minimum is occurring.
* @param q Array whose first dimension is the length of the query time series and the second dimension is the number of
* queries.
* @param t Array whose first dimension is the lenght of the time series and the second dimension is the number of time
Copy link
Contributor

Choose a reason for hiding this comment

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

length (check other copies of this doc)

@@ -45,6 +112,40 @@ bool tileIsFarFromDiagonal(long bandSize, long numRows, long row, long numColumn
*/
af::array generateMask(long m, long numRows, long row, long numColumns, long column, long nTimeSeries = 1);

/**
* @brief Calculates the Mueen distance.
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this to calculate the Mueen similarity search. See #107. When this PR gets merged, please close the mentioned issue.

Some refactor has been performed in order to publish in the API the mass
function not requiring some intermediate computation as parameters.

The function findBestNOccurrences has been also added to the API. The C
and JNI bindings have been also included for these two functions

The corresponding unit tests have been created.
@jrecuerda jrecuerda force-pushed the feature/findBestNOccurrences branch from d7b4c4b to a1f5daa Compare June 4, 2019 08:03
Copy link
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

Copy link
Contributor

@avilchess avilchess left a comment

Choose a reason for hiding this comment

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

LGTM

@jrecuerda jrecuerda merged commit cf7836a into master Jun 4, 2019
@otorreno otorreno deleted the feature/findBestNOccurrences branch June 4, 2019 09:54
@jrecuerda jrecuerda mentioned this pull request Jun 4, 2019
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.

3 participants