Lda opt #99
Lda opt #99
Conversation
Update master branch in the forked repo
hyu596
force-pushed
the
lda_opt
branch
4 times, most recently
from
e319875 to
767aeb5
Compare
Sync with master
|
|
||
| return config | ||
|
|
||
| def launch_ps(self, command_dict=None): |
There was a problem hiding this comment.
This file needs some work to improve a few things:
- The lambdas should be kept alive by the automate.py (as is done in the other algorithms).
- The parameter server should be launched by the automate.py (see logistic_regression,ipynb)
src/Configuration.h
Outdated
| }; | ||
|
|
||
| static constexpr int WORKERS_BASE[5] = {-1, 3, -1, -1, 4}; |
src/LDAModel.cpp
Outdated
| K_ = load_value<int16_t>(info); | ||
|
|
||
| // load the current topic assignments | ||
| t.clear(); |
There was a problem hiding this comment.
Because this is the constructor, t/d/w are empty vectors. No need for clear.
src/Tasks.h
Outdated
| std::vector<std::vector<int>>& topic_scope); | ||
|
|
||
| private: | ||
| std::array<int, VOCAB_DIM_UPPER> lookup_map; |
There was a problem hiding this comment.
This is a bit dangerous. If VOCAB_DIM_UPPER is too big it can lead to crashes be cause std::array puts data on the stack.
You can replace this with an std::vector and then resize to VOCAB_DIM_UPPER.
There was a problem hiding this comment.
This happens in other places. We can fix this later.
There was a problem hiding this comment.
lookup_map is now vector and its initializing part (filling with -1) is in the beginning of LoadingLDATaskS3.run function.
src/LoadingLDATaskS3.cpp
Outdated
| // Storing local variables (LDAStatistics) | ||
| for (unsigned int i = 1; i < num_s3_objs + 1; ++i) { | ||
| // for (unsigned int i = 1; i < 3; ++i) { | ||
| std::vector<int> w; |
There was a problem hiding this comment.
Move this to right before count_dataset().
| float est_time_one_iter = | ||
| ((get_time_ms() - start_time) / 1000.) / full_iteration; | ||
|
|
||
| if (elapsed_sec > (lambda_time_out - est_time_one_iter - 10.)) { |
There was a problem hiding this comment.
Too much code here. Would put this code below in a separate function.
There was a problem hiding this comment.
Code refactoring has been done. Please let me know if it remains messy.
| } | ||
| } | ||
| int since_start_sec = (get_time_ms() - start_time) / 1000; | ||
| if (since_start_sec > benchmark_time) { |
There was a problem hiding this comment.
Same thing, this should go into separate function
There was a problem hiding this comment.
^^Code refactoring has been done. Please let me know if it remains messy.
| */ | ||
| void load_serialized_indices(char* mem_begin); | ||
|
|
||
| std::vector<std::unique_ptr<std::thread>> help_upload_threads; |
There was a problem hiding this comment.
It's being used now [1]. Previously help_upload_threads was removed since in all the experiments I ensured that each worker was assigned with exactly one LDAStatistics and thus it was not needed.
[1] https://github.com/hyu596/cirrus-1/blob/lda_opt/src/LDATaskS3.cpp#L201
There was a problem hiding this comment.
Work has been done to ensure it works fine.
| uint64_t to_send_size, | ||
| int& upload_lock, | ||
| int bucket_id) { | ||
| while (true) { |
There was a problem hiding this comment.
Don't understand this code. As far as I can understand this task is single threaded (help_upload_threads is not used).
bootstrap.sh
Outdated
| fi | ||
| cd lz4-dev | ||
| make | ||
| make install |
There was a problem hiding this comment.
Right now the LDA tests are failing on travis because we don't have permissions to do 'make install' there. To fix this do:
- Remove this make install and sudo make install.
- Add to the .travis.yml file an "apt-get install liblz4-dev".
- Add liblz4-dev to the cirrus/README
This PR implements distributed Latent Dirichlet Allocation using the Cirrus system and interface.