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

thread pool for finding bigrams #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

thread pool for finding bigrams #1

wants to merge 2 commits into from

Conversation

SuperDoxin
Copy link

it automatically uses a nice number of processes, or you can specify with --threads.

it doesn't output the same header, but I don't know if this is a regression or if it doesn't matter. haven't the foggiest on how to test for that though.

@Ed-von-Schleck
Copy link
Owner

Thanks a lot! I'll look at it in greater detail the next days (and try to find out why the result differs), but here's some little things I noticed from glancing over it:

  • Try to massage the changes a bit into PEP8 style, with the exception that long code lines are allowed. Specifically, please insert spaces after commas and around operators like = (except for default or keyword arguments).
  • multiprocessing.pool() creates a process pool. While there's technically seperate threads involved, it isn't wrong, but a bit misleading to call the argument --threads. Please rename that (and variable names too) to something with processes.

The biggest time consumer is the optimization of the encoding packs, and that's what would profit most of multiprocessing, so if you like, you might want to tackle that, too – that would really be awesome.

Again, thanks a lot, this is a very welcome contribution!

@SuperDoxin
Copy link
Author

I stripped out some ordered dicts with normal dicts since the threads can finish in any order. I hoped it wouldn't matter. turns out it might. I'd like to know if it's a problem before I go finish the code up some more.

I'm a terrible terrible person for not following pep8 ;_;

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