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

Added DTM example notebook #471

Merged
merged 3 commits into from Oct 16, 2015
Merged

Added DTM example notebook #471

merged 3 commits into from Oct 16, 2015

Conversation

Arttii
Copy link
Contributor

@Arttii Arttii commented Sep 28, 2015

Hi,

Finally got around to doing this. I applied some pep8 to this and rearranged some stuff. Tell me if we need to change anything else.

Artyom

@piskvorky
Copy link
Owner

The example dataset is already processed into list of tokens.

What "the" dataset? I don't see any dataset anywhere -- what am I missing?

It would be more instructive to show what kind of input DTM accepts. It looks like the notebook unpickles some file from a local disk (?) now, but I (the user) still have no idea what that is.

Can you think of a way to make this clearer?

Thanks!

@Arttii
Copy link
Contributor Author

Arttii commented Sep 28, 2015

Ya it just basically loads up a normal gensim corpus + a time sequence array. Do you think embedding the pickled file inside the notebook will make it more clear? Its like basically an list of lists.

I changed it a bit. Check if it's more understandable now.

@piskvorky
Copy link
Owner

Yes, much better, thanks :)

I can't see any output for the last three notebook cells though -- is that on purpose?

@tmylk can you help @Arttii with cleaning up this PR? Cheers.

@Arttii Arttii force-pushed the develop branch 2 times, most recently from b01cbda to 12a5706 Compare September 29, 2015 10:31
@tmylk
Copy link
Contributor

tmylk commented Sep 30, 2015

Hey @Arttii , thanks for this. I ran it on my Fedora Linux no problem.
Some minor corrections in this version. If you are ok with them then please add to this PR https://github.com/piskvorky/gensim/blob/9c8e784d7a280744312751b3c9b41611798b49d1/docs/notebooks/dtm_example.ipynb

@piskvorky
Copy link
Owner

@tmylk still some typos ("defentiion") and PEP8 nitpicks (space after comma, line break around DTM_HOME), but it's looking up! Almost ready for merge.

Btw I've seen a message on the mailing list by @deakkon that it doesn't work, even with the initialize_lda=True hack.

@Arttii @tmylk I'd prefer to investigate and fix this thoroughly, before merging the tutorial.

@tmylk
Copy link
Contributor

tmylk commented Oct 5, 2015

Unfortunately I couldn't reproduce @deakkon's mailing list issue on my Linux box.

The wrapper works well for me on Linux.

PEP8 and spelling updated in:
https://github.com/piskvorky/gensim/blob/Arttii-develop/docs/notebooks/dtm_example.ipynb

@Arttii
Copy link
Contributor Author

Arttii commented Oct 5, 2015

I'll have you to check that. Sorry haven't had time recently. I'll take a look tomorrow maybe.


From: Lev Konstantinovskiymailto:notifications@github.com
Sent: ‎10/‎5/‎2015 19:52
To: piskvorky/gensimmailto:gensim@noreply.github.com
Cc: Arttiimailto:artyom.topchyan@live.com
Subject: Re: [gensim] Added DTM example notebook (#471)

Unfortunately I couldn't reproduce @deakkon's mailing list issue on my Linux box.

The wrapper works well for me on Linux.

PEP8 and spelling updated in:
https://github.com/piskvorky/gensim/blob/Arttii-develop/docs/notebooks/dtm_example.ipynb


Reply to this email directly or view it on GitHub:
#471 (comment)

@tmylk
Copy link
Contributor

tmylk commented Oct 13, 2015

Hey @Arttii, is this ok to merge?
In PR #476 I added error forwarding via subprocess.check_output to DTM and Mallet wrappers so there would be less mailing list queries in the future.

@Arttii
Copy link
Contributor Author

Arttii commented Oct 13, 2015

Hi,

Looks good. Sorry I didn't help out more some hectic work stuff.

Artyom


From: Lev Konstantinovskiymailto:notifications@github.com
Sent: ‎10/‎13/‎2015 10:31
To: piskvorky/gensimmailto:gensim@noreply.github.com
Cc: Arttiimailto:artyom.topchyan@live.com
Subject: Re: [gensim] Added DTM example notebook (#471)

Hey @Arttii, is this ok to merge?
In PR #476 I added error forwarding via subprocess.check_output to DTM and Mallet wrappers so there would be less mailing list queries in the future.


Reply to this email directly or view it on GitHub:
#471 (comment)

tmylk added a commit that referenced this pull request Oct 16, 2015
Added DTM example notebook
@tmylk tmylk merged commit 3bdf64c into piskvorky:develop Oct 16, 2015
@tmylk
Copy link
Contributor

tmylk commented Oct 16, 2015

@Arttii Thanks a lot for adding the DTM wrapper! It seems to be used by a lot of people now.

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.

None yet

3 participants