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

fix scoring without __prepare_items, add unit test #417

Merged
merged 1 commit into from
Aug 2, 2015
Merged

fix scoring without __prepare_items, add unit test #417

merged 1 commit into from
Aug 2, 2015

Conversation

mataddy
Copy link
Contributor

@mataddy mataddy commented Aug 2, 2015

This fixes the problems pointed out in #407, which occurred due to a refactoring of the Train methods.

I went through the new train methods and adapted things related to score() accordingly. Re-running my standard yelp example looks fine: http://nbviewer.ipython.org/github/taddylab/deepir/blob/master/deepir.ipynb

Also added a testScoring() so that we won't be suprised by this type of thing again.

Thanks!

@piskvorky
Copy link
Owner

Alright! The test is rather superficial, but it will do for now :)

Merging. Thanks @mataddy !

piskvorky added a commit that referenced this pull request Aug 2, 2015
fix scoring without __prepare_items, add unit test
@piskvorky piskvorky merged commit c364c33 into piskvorky:develop Aug 2, 2015
@mataddy
Copy link
Contributor Author

mataddy commented Aug 2, 2015

no prob, thank you. yep, the test is a bit cheeky I'll update it I promise ;-)

@gojomo
Copy link
Collaborator

gojomo commented Aug 4, 2015

That test would have saved me from inadvertently breaking this – sorry! – so it's a great start in my view. :)

@mataddy
Copy link
Contributor Author

mataddy commented Oct 23, 2015

hi @piskvorky, sorry for the long delay but there is finally a more introductory demo up at https://github.com/TaddyLab/deepir/blob/master/deepir.ipynb. the docprob wrapper function in the demo is something I could add to gensim if you're into it.

@piskvorky
Copy link
Owner

Looks nice, thanks Matt!

Do we integrate this notebook into the docs/notebooks directory of gensim, or just link to it somehow?

I won't have time to review in detail until November; CC @tmylk.

@mataddy
Copy link
Contributor Author

mataddy commented Oct 24, 2015

great idea; so long as those docs/notebooks aren't run when the project is built this will work great. I'll create a pull request with it and ping @tmylk

@gojomo
Copy link
Collaborator

gojomo commented Oct 24, 2015

Hmm, testing the notebooks each build is an interesting idea... but for now, they're not being run/tested every build.

@mataddy
Copy link
Contributor Author

mataddy commented Oct 24, 2015

cool, thanks @gojomo. FYI the only reason it wouldn't work for my notebook here is because the data is on kaggle and the user needs to sign-up to get it. but that could be worked around later if you guys decide to change policies and run the notebooks at each build

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