Refactor SCRIMP++

[tylerwmarrs]

The SCRIMP++ module is a little difficult to follow. It should be refactored once we establish some code style guidelines. #26

[ameya98]

I think I might be able to make some progress on this.

[ameya98]

I did make progress on my fork. I also fixed a bug in SCRIMP++ where the matrix profile indices were taking invalid values. I also added comments in some places, and removed some variables that weren't necessary.

[ameya98]

There was another bug where the exclusion zones were not applied to the distance profiles. This has been fixed - SCRIMP++ now gives me the same output as STOMP.

[ameya98]

I want to make a pull request with all of my contributions and bug fixes, but I think I've added quite a few things (exclusion_zone_fraction) and modified the names of other variables, as well as committed to my master branch only. What do you think? Someone could benefit from these fixes though.

[tylerwmarrs]

@ameya98 Typically, it is best practice to solve one issue in a branch in your repository and then submit a pull request. You may be able to use git's cherry pick functionality to create new branches that resolve specific issues.

An an aside, my implementation followed UCR Matlab code explicitly where as the stomp implementation in this library was based on the paper. I'm not sure that it makes sense to have SCRIMP++ follow the stomp standards. @vanbenschoten what do you think?

[ameya98]

It's not about the standards, it's about the implementation here: both STOMP and SCRIMP++ (when run to completion) compute the exact matrix profile. There is no reason for the outputs of both of these to be any different (except for floating point errors, but those errors are extremely minor).
The current implementation has bugs - the matrix profile indices were being incorrectly computed (the indices were taking values larger than the profile length), and the exclusion zone should be applied when computing the distance profiles in SCRIMP (just like you would do in STOMP).

[ameya98]

Correction on the exclusion zones - there shouldn't be any need for them in PreSCRIMP. There seems to be some issue with the noise correction I've implemented.

[tylerwmarrs]

@ameya98 It is great that you are helping resolve some of these issues. When you find a new bug unrelated to refactoring, please create a new issue. When you submit a pull request please reference the issue in the pull request. Please review the contributing guidelines. https://github.com/target/matrixprofile-ts/blob/master/CONTRIBUTING.md

[ameya98]

Sorry, I've done so now!