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

Collaborative filtering package should expect zero-indexed users and items #311

Closed
rcurtin opened this Issue Dec 29, 2014 · 4 comments

Comments

Projects
None yet
1 participant
@rcurtin
Member

rcurtin commented Dec 29, 2014

Reported by rcurtin on 9 Oct 44101854 14:08 UTC
Currently, the CF class expects users and locations to be one-indexed; that is, the user and items IDs never take the value 0. However this doesn't really make sense in a programming sense because C++ is a zero-indexed language. So, the CF class (and its documentation) must be updated so that it does not assume that 1 is the smallest index; instead, the assumption should be that 0 is the smallest index.

This problem manifests itself mainly in CleanData(), where if you pass '0' as a user or item, the method will fail (try it and you'll see what I mean). This was the cause of ticket #232.

@rcurtin rcurtin self-assigned this Dec 29, 2014

@rcurtin rcurtin added this to the mlpack 1.0.9 milestone Dec 29, 2014

@rcurtin rcurtin closed this Dec 29, 2014

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Dec 30, 2014

Member

Commented by siddharth.950 on 20 Mar 44102833 12:20 UTC
Shouldn't the support be independent of the programming language? Or is most of the data zero-indexed? Anyway, I'm attaching a diff file. Tell me if that fixes it.

Member

rcurtin commented Dec 30, 2014

Commented by siddharth.950 on 20 Mar 44102833 12:20 UTC
Shouldn't the support be independent of the programming language? Or is most of the data zero-indexed? Anyway, I'm attaching a diff file. Tell me if that fixes it.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Dec 30, 2014

Member

Commented by rcurtin on 27 Jun 44102936 18:04 UTC
Have you tested this using mlpack_test? Does it affect the CF test results?

Member

rcurtin commented Dec 30, 2014

Commented by rcurtin on 27 Jun 44102936 18:04 UTC
Have you tested this using mlpack_test? Does it affect the CF test results?

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Dec 30, 2014

Member

Commented by rcurtin on 3 Jun 44106046 06:03 UTC
Patch applied in r16235. The documentation still needs to be updated (or created) though.

Member

rcurtin commented Dec 30, 2014

Commented by rcurtin on 3 Jun 44106046 06:03 UTC
Patch applied in r16235. The documentation still needs to be updated (or created) though.

@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Dec 30, 2014

Member

Commented by rcurtin on 23 Jul 44117090 01:24 UTC
Patch applied in r16282.

Member

rcurtin commented Dec 30, 2014

Commented by rcurtin on 23 Jul 44117090 01:24 UTC
Patch applied in r16282.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment