add cython with alternative indexing approach

[esquires]

Pull request #4 ended with the decision to go with an alternative indexing method that was faster. This pull request contains the indexing method. See pull request #4 for a speed comparison of the 2 methods.

[pipitang]

Really excellent work. Just eager to know when this will be merged into the release version?

Thanks.

[slaypni]

I have merged and uploaded it to PyPI! @esquires Thank you for writing the code. fastdtw got significant performance boost thanks to you! @kernc Thank you for advising a lot. Your comments were all very helpful!

[kernc]

You can now sqush the commits before merging on GitHub. One reason for this, in particular, is that contributions still show (in repo history and in visualizations on the Graphs tab). It's common courtesy to leave commit's Author field intact, obviously. :smiley:

[slaypni]

I run git merge --squash esquires-indexing-approach on command line. I was wondering why merged icon was not shown on this page. Should I just run git merge --no-ff esquires-indexing-approach?

[slaypni]

Maybe there are better ways to squash commits on command line? Otherwise, pressing GitHub's merge button is the good way?

[slaypni]

I have reset commits, rebased commits of this PR and force pushed. Now @esquires is displayed in Graphs tab. :-)

[slaypni]

@esquires fastdtw._fastdtw.dtw() fails when dist is int. For instance, dtw_c([[1,2]], [[1,1], [2,2]], dist=1) throw an error.

TypeError: unsupported operand type(s) for -: 'list' and 'list'

Do you come up with any idea to fix it?

[esquires]

The latest has dtw calling the same prep code as fastdtw. It also converts lists to numpy arrays when using a p-norm.

[slaypni]

@esquires Thank you for writing the code! Could you open a new PR which derives from latest slaypni/master for the change?

[kernc]

No need to open a new PR. @esquires, just rebase the changes on the latest (force pulled) master. Since recently, it's also possible to switch the base branch of a PR.

[esquires]

I cherry-picked/squashed 3043419 and ae41de1 onto master and fixed the merge conflicts. All the tests ran fine. Apparently one can only change the base branch, not the branch containing the new code so I am just leaving it in master for now without updating the pull request branch. If you want me to open a new pull request let me know. The commit with the fix is in 6375aef.

[kernc]

Something like that didn't work?

# Add upstream remote repo as upstream
git remote add upstream git@github.com:slaypni/fastdtw.git
# Force-fetch its master branch
git fetch -f upstrem master

# Switch to the branch of the PR
git checkout indexing-approach
# Backup into a new branch, just in case
git checkout -b indexing-approach-bak
# Switch back to PR branch
git checkout -

# Rebase the changes the branch introduces 
git rebase -i -X theirs upstream/master
# Force-push the new branch
git push -f

[esquires]

Hm. I must have misunderstood your request. I thought you wanted me to change the pull request to point to esquires/master. I'll just have indexing-approach point to 6375aef.

[esquires]

indexing-approach now points to 6375aef

[slaypni]

Thunk you guys! I have merged it.

[slaypni]

@esquires BTW, I found another bug. _fastdtw.fastdtw returns a wrong result when x and y are 2-dimensional array and dist is None. For instance,

> fastdtw_c([[1,2]], [[2,2],[1,1]])
(0.0, [(0, 0), (0, 1)])

Pure python's fastdtw.fastdtw causes an error properly.

> fastdtw_p([[1,2]], [[2,2],[1,1]])
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

Do you find a way to fix it? (If you could write a code for fixing it, please open a new PR since this PR is already merged) 😃