{"action":"created","issue":{"url":"https://api.github.com/repos/biota/sourcetracker2/issues/34","repository_url":"https://api.github.com/repos/biota/sourcetracker2","labels_url":"https://api.github.com/repos/biota/sourcetracker2/issues/34/labels{/name}","comments_url":"https://api.github.com/repos/biota/sourcetracker2/issues/34/comments","events_url":"https://api.github.com/repos/biota/sourcetracker2/issues/34/events","html_url":"https://github.com/biota/sourcetracker2/issues/34","id":149017775,"number":34,"title":"refactor discussion / what I think we need before release","user":{"login":"wdwvt1","id":1048569,"avatar_url":"https://avatars.githubusercontent.com/u/1048569?v=3","gravatar_id":"","url":"https://api.github.com/users/wdwvt1","html_url":"https://github.com/wdwvt1","followers_url":"https://api.github.com/users/wdwvt1/followers","following_url":"https://api.github.com/users/wdwvt1/following{/other_user}","gists_url":"https://api.github.com/users/wdwvt1/gists{/gist_id}","starred_url":"https://api.github.com/users/wdwvt1/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wdwvt1/subscriptions","organizations_url":"https://api.github.com/users/wdwvt1/orgs","repos_url":"https://api.github.com/users/wdwvt1/repos","events_url":"https://api.github.com/users/wdwvt1/events{/privacy}","received_events_url":"https://api.github.com/users/wdwvt1/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2016-04-18T01:38:55Z","updated_at":"2016-04-18T12:03:43Z","closed_at":null,"body":"@gregcaporaso @justin212k @lkursell @johnchase @nscott1 @mjk1000\r\n\r\nBased on working with the code this weekend to try and get @johnchase and @lkursell a simple API for launching jobs I have come to the conclusion that the code has accumulated some cruft. Cruft = technical debt that will come due at some time, so I think we need to consider refactoring. I am not advocating something major, but a set of suggestions that @lkursell and @johnchase have already essentially asked for. \r\n\r\nBelow are my thoughts, please let me know what you think\r\n\r\n**The things I think would help and plan on doing once we modify/come to agreement**\r\n\r\n1. Refactoring all functions that are not the `gibbs_sampler` to use `pd.DataFrame`'s. A significant amount of code is spent in `sourcetracker/sourcetracker.py` as well as `sourcetracker/cli/gibbs.py` to ensure that arrays representing source data, sink data, and the mapping file data are in the proper order etc. `DataFrame`s can help remove much of this code (@lkursell and @johnchase pointed this out). This could include converting input `biom` tables to a `DataFrame` as it would make some operations slightly easier. No necessity of this, but a possibility for a little extra gain. \r\n \r\n2. Rewriting `ConditionalProbability` to be clearer and more easily extensible. In our efforts to eke out speed improvements I wrote this class so that it would precompute everything that could be precomputed. The speed gains from this are minor based on my current understanding, and it makes understanding whats happening much less straightforward. In addition, as we move to add other layers of probability (e.g. what @lkursell) discussed in his last email, we will likely want to add them in this class. Without refactoring, it's going to be hell trying to do this. \r\n\r\n3. Visualization needs to come with basic call I think (though perhaps leaving it in the ipython notebook is okay). It will take very little work to create a set of simple visualizations and this will make people switching to this code much more likely. \r\n\r\n**What I need help with**\r\n\r\n1. I'd love to get rid of the `functools.partial` nonsense that we are using to enable passing jobs to an ipyparallel client. I don't think we should need to do this and it would get rid of a fair amount of code, but I don't understand why we need to do this in the first place. If you try and pass a function to the engines that is not wrapped by `partial` you get errors indicating the local namespace doesn't contain the variables you are trying to pass. \r\n\r\n**Things I'd like to get a consensus on**\r\n\r\n1. Currently we are asking each engine in a cluster to operate on a single sink and write the results to an intermediate file. We did this to allow easy recovery from a failure (if you are running ST2 on 500 samples and it quits at sample 485 you'd be able to save that 97% of the work) but it means more functions and more intermediate text files. Do people think it would be better to just store data in memory and write out a final result when every computation had been completed? @lkursell has suggested that the in-memory approach might be good as he has had no mid-run failures in his entire time using this code.   "},"comment":{"url":"https://api.github.com/repos/biota/sourcetracker2/issues/comments/211350715","html_url":"https://github.com/biota/sourcetracker2/issues/34#issuecomment-211350715","issue_url":"https://api.github.com/repos/biota/sourcetracker2/issues/34","id":211350715,"user":{"login":"ajaykshatriya","id":7661826,"avatar_url":"https://avatars.githubusercontent.com/u/7661826?v=3","gravatar_id":"","url":"https://api.github.com/users/ajaykshatriya","html_url":"https://github.com/ajaykshatriya","followers_url":"https://api.github.com/users/ajaykshatriya/followers","following_url":"https://api.github.com/users/ajaykshatriya/following{/other_user}","gists_url":"https://api.github.com/users/ajaykshatriya/gists{/gist_id}","starred_url":"https://api.github.com/users/ajaykshatriya/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajaykshatriya/subscriptions","organizations_url":"https://api.github.com/users/ajaykshatriya/orgs","repos_url":"https://api.github.com/users/ajaykshatriya/repos","events_url":"https://api.github.com/users/ajaykshatriya/events{/privacy}","received_events_url":"https://api.github.com/users/ajaykshatriya/received_events","type":"User","site_admin":false},"created_at":"2016-04-18T12:03:42Z","updated_at":"2016-04-18T12:03:42Z","body":"Hi Greg:  My general view is spending X hours updating  ST2  with new or improved functionality should yield at least 2X hours in increased efficiency of data analysis per person in the next 90 days. I defer to the data scientists on whether aforementioned changes would yield such a boost. \n\nIf so, it’s worth your limited and highly-valued time to make these updates.\n\nBest,\nAjay\n------------------------------\nAjay Kshatriya\nChief Executive Officer\nBiota Technology, Inc.\n+1-650-888-6512\n\nConfidentiality Statement: This e-mail message, and all attachments, may contain legally privileged or confidential information intended solely for the use of the individual, agent, or entity named in the e-mail. If the reader of this message is not the intended recipient, you are hereby notified that any reading, distribution, copying, or taking of action based on its contents is strictly prohibited. If you have received this e-mail in error, please notify the sender immediately and delete this message from your system.\n\n> On Apr 18, 2016, at 4:50 AM, Greg Caporaso <notifications@github.com> wrote:\n> \n> @wdwvt1 <https://github.com/wdwvt1>, I would be happy to work with you on some of these things to make it go faster.\n> \n> For the point about temp files, is the issue that they're cluttering an output directory? In that case, it might be possible to instead have them written to a temporary directory (using Python's tempfile module). It seems like if you have failure recovery built in already we might not want to remove it.\n> \n> You might need the partial stuff - I'd have to spend a little time looking at it to do that.\n> \n> My plan is to spend about 3 hours / month on ST2 maintenance, and I haven't done that yet this month, so I have some free time to work with you on this if you'd like. I could then do the release in early May. I would just check with @ajaykshatriya <https://github.com/ajaykshatriya> to see if he'd like to see these changes go in as well.\n> \n> —\n> You are receiving this because you were mentioned.\n> Reply to this email directly or view it on GitHub <https://github.com/biota/sourcetracker2/issues/34#issuecomment-211344593>\n\n"}}