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

Adds stringsimmatrix function #79

Merged
merged 3 commits into from
Nov 3, 2019
Merged

Conversation

JBGruber
Copy link
Contributor

Hi @markvanderloo and the other contributors,

Thanks for this great package!

I use it in one of my packages where it really solved an issue for me. However I want to re-write the function that is built on top of stringdist at some point in the future and thought about what I need for that.

A stringsimmatrix function was what came to mind and rather than putting it into my package I looked into your code and saw that it is rather easy to accomplish that.

The new function is basically all your code from stringsim and stringdistmatrix rearranged.

@JBGruber JBGruber changed the title Added stringsimmatrix function Adds stringsimmatrix function Oct 31, 2019
@coveralls
Copy link

coveralls commented Oct 31, 2019

Coverage Status

Coverage decreased (-0.01%) to 89.056% when pulling 6874d59 on JBGruber:master into dc4157a on markvanderloo:master.

@markvanderloo
Copy link
Owner

Hi Johannes,

this looks like a great contribution, thanks! Before I merge, could you add just a few tests to inst/tinytest/test_stringsim.R? Maybe just check dimensions and compare a simple matrix with output computed 'manually' with stringsim.

I know the code is quite short, but adding tests will give some protection if we somehow need to update one of the functions that is used by stringsim.

Best,
Mark

@JBGruber
Copy link
Contributor Author

JBGruber commented Nov 2, 2019

Hi Mark,

Sure! I wasn't familiar with your tinytest package and wanted to wait if there is a general interest in the PR before looking into how to write some tests. But it's actually very straightforward.

I used a character vector from a built-in dataset (islands) to calculate some real similarities. I think that makes sense in this case but I can also copy your approach from testing stringdistmatrix (I only had the idea after already writing the other tests). Let me know what you prefer.

I don't know why coveralls stills shows that coverage decreases. As far as I can see all my code is covered.

Best
Johannes

@markvanderloo markvanderloo merged commit c42de9d into markvanderloo:master Nov 3, 2019
@markvanderloo
Copy link
Owner

Thanks! accepted.

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.

3 participants