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

bug fix in randindex #99

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

bug fix in randindex #99

wants to merge 1 commit into from

Conversation

trappmartin
Copy link

RandIndex should be 1. of both assignments are equal. The current code returns 0.

@kmsquire
Copy link
Contributor

Hi @trappmartin, thanks for the contribution. Can you give an example of where the current code is incorrect and your code is correct? I'm having trouble creating one.

If I have, e.g.

c1 = [1]
c2 = [1]

then n == 1, but nc == nan, so t1 != nc, and the test on line 27 fails, and produces a nan result... so it seems that the code is still incorrect?

@kmsquire
Copy link
Contributor

Also, could you also add tests which cover all branches of your changes (once you've verified that they're correct)?

@trappmartin
Copy link
Author

Interesting, I haven't checked this case.
On my machine, the current code produces 0 if

c1 = [1, 2, 3]
c2 = [1, 2, 3]

which should not be the case.
I'll check the case you reported and add a fix asap (after the weekend).

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.

2 participants