-
Notifications
You must be signed in to change notification settings - Fork 14
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
ISSUE-177 Parallelize SAM algorithm in mineraly.py using Joblib #181
Conversation
@aheermann thanks for opening this PR. Please resolve above conflicts. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @aheermann thanks for posting your PR. Please consider and address the comments in my review.
@aheermann this is much better. Thanks for updating. |
Excellent @aheermann this looks much better. |
I'm going to pull the code locally @aheermann and test. Please standby before merging thank you. Excellent job. |
@capstone-coal/19-usc-capstone-team please be in a position to discuss and compare results of this review by tomorrows meeting. Thank you all in advance. |
@aheermann please update this PR to resolve conflict. @capstone-coal/19-usc-capstone-team has anyone else tested this PR out? |
During our meeting I believe @aheermann mentioned looking into the issues involving the memory leak with joblib. He mentioned that there is a known issue within the library. |
@aheermann is this ready too be tested again then? Thanks for pushing most recent commit. |
PING here... do you have a status update please? |
@lewismc, this should have fixed the memory leak, as long as you make sure you have psutil installed. Past that, I imagine this branch will not be merged to master, but instead be combined with the pytorch one, for the config file branch. |
@aheermann I'll test this right now and provide feedback.
No let's merge this one first. We can then update #199 and finally #198 |
BTW, installation on < python: stable 3.7.5 resulted in failure. Upgrading to Python 3.7.5 fixed that. Just in case anyone tries. |
Hi @aheermann this has fixed the issue. See below for log
Excellent work here. If we take into consideration that the initial processing time was well over a day... or maybe even longer, then this represents a huge speedup. I'm going to go ahead and merge this into master branch. Excellent work. |
Adds joblib functionality to mineral.py as specified in #177. Will be one possible parallelization option that can be specified in config file