{"action":"created","issue":{"url":"https://api.github.com/repos/KlugerLab/FIt-SNE/issues/20","repository_url":"https://api.github.com/repos/KlugerLab/FIt-SNE","labels_url":"https://api.github.com/repos/KlugerLab/FIt-SNE/issues/20/labels{/name}","comments_url":"https://api.github.com/repos/KlugerLab/FIt-SNE/issues/20/comments","events_url":"https://api.github.com/repos/KlugerLab/FIt-SNE/issues/20/events","html_url":"https://github.com/KlugerLab/FIt-SNE/pull/20","id":336520621,"node_id":"MDExOlB1bGxSZXF1ZXN0MTk3OTUxODY5","number":20,"title":"Refactor interpolation code","user":{"login":"pavlin-policar","id":5758119,"node_id":"MDQ6VXNlcjU3NTgxMTk=","avatar_url":"https://avatars2.githubusercontent.com/u/5758119?v=4","gravatar_id":"","url":"https://api.github.com/users/pavlin-policar","html_url":"https://github.com/pavlin-policar","followers_url":"https://api.github.com/users/pavlin-policar/followers","following_url":"https://api.github.com/users/pavlin-policar/following{/other_user}","gists_url":"https://api.github.com/users/pavlin-policar/gists{/gist_id}","starred_url":"https://api.github.com/users/pavlin-policar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pavlin-policar/subscriptions","organizations_url":"https://api.github.com/users/pavlin-policar/orgs","repos_url":"https://api.github.com/users/pavlin-policar/repos","events_url":"https://api.github.com/users/pavlin-policar/events{/privacy}","received_events_url":"https://api.github.com/users/pavlin-policar/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-06-28T08:21:55Z","updated_at":"2018-07-04T18:54:56Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/KlugerLab/FIt-SNE/pulls/20","html_url":"https://github.com/KlugerLab/FIt-SNE/pull/20","diff_url":"https://github.com/KlugerLab/FIt-SNE/pull/20.diff","patch_url":"https://github.com/KlugerLab/FIt-SNE/pull/20.patch"},"body":"For the past month or two, I've been playing around with this method, and in order to better understand how it works, I've rewritten most of the interpolation code, as it was quite confusing.\r\n\r\n**Description of changes**\r\n\r\nMy primary goal here was to make the code understandable to anybody following your paper.\r\n\r\nThe first thing I did was run formatting on the project, as it really was quite inconsistent and that really bugs me :) Unfortunately, this makes the entire PR quite large, having touched every file in the project.\r\n\r\nThe files I did change and that should be reviewed are `nbodyfft` (I rewrote most of this) and `tsne` (I didn't really do much here, changed a few things in the parts that relate to the FFT bit.\r\n\r\nI noticed that there was a lot of seemingly needless memory rearranging via `iarr`, `ysort` and the likes. After a bit of head scratching, I found that the code basically ensures that points belonging to the same box (or interval) are rearranged to be close to each other in memory. This really isn't necessary, because we can compute the correct interval index on the fly, saving on a lot of memory allocation and rearraging. I removed all of that code and comptued the indices on the fly. IMO this makes the function a whole lot clearer and understandable.\r\n\r\nThe indices are a bit complicated to compute e.g.\r\n\r\n```\r\nint idx = (box_i * n_interpolation_points + interp_i) * (n_boxes * n_interpolation_points) +\r\n          (box_j * n_interpolation_points) + interp_j;\r\n```\r\n\r\nbut if you think about it a bit, it is easier to get your head around than the previous rearranging. I just took a piece of paper and drew a grid to figure these out and it's really quite easy. This also has the added benefit of being a bit faster than the previous version, since we're not wasting time copying memory around.\r\n\r\nOther than that I made the code more C++ish (under the guidance of intelliJ Clang-tidy warnings. I am by no means good at C++ so change things at will) and added comments that I thought would make the code clearer to somebody following your paper. I also removed a lot of commented out code that was presumably there for debugging, as it was taking up a lot of space and making everything very unreadable.\r\n\r\nI also added a CMakeLists.txt as per the recommendation of intelliJ. I think that's really it, but as I have been working on this for a while, I may have forgotten to mention some small thing.\r\n\r\n**End note**\r\n\r\nI was fairly careful that the results I got were always still correct, but one can never be too careful. If you find any bugs or issues, I'll be sure to correct them.\r\n\r\nAlso, I'd ask you make sure that my comments are in fact correct, as understanding the code was my primary goal here and making it understandable for anybody going through it in the future. A misleading comment is worse than no comment at all. Same goes for variable names.\r\n\r\nAs requested, I rebased my branch on top of the latest master so you guys wouldn't have to deal with the many merge conflicts. I haven't had the chance to test the functionality you guys added in the past two months. It should work, but it should be tested."},"comment":{"url":"https://api.github.com/repos/KlugerLab/FIt-SNE/issues/comments/402542955","html_url":"https://github.com/KlugerLab/FIt-SNE/pull/20#issuecomment-402542955","issue_url":"https://api.github.com/repos/KlugerLab/FIt-SNE/issues/20","id":402542955,"node_id":"MDEyOklzc3VlQ29tbWVudDQwMjU0Mjk1NQ==","user":{"login":"linqiaozhi","id":19375944,"node_id":"MDQ6VXNlcjE5Mzc1OTQ0","avatar_url":"https://avatars1.githubusercontent.com/u/19375944?v=4","gravatar_id":"","url":"https://api.github.com/users/linqiaozhi","html_url":"https://github.com/linqiaozhi","followers_url":"https://api.github.com/users/linqiaozhi/followers","following_url":"https://api.github.com/users/linqiaozhi/following{/other_user}","gists_url":"https://api.github.com/users/linqiaozhi/gists{/gist_id}","starred_url":"https://api.github.com/users/linqiaozhi/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/linqiaozhi/subscriptions","organizations_url":"https://api.github.com/users/linqiaozhi/orgs","repos_url":"https://api.github.com/users/linqiaozhi/repos","events_url":"https://api.github.com/users/linqiaozhi/events{/privacy}","received_events_url":"https://api.github.com/users/linqiaozhi/received_events","type":"User","site_admin":false},"created_at":"2018-07-04T18:54:56Z","updated_at":"2018-07-04T18:54:56Z","author_association":"MEMBER","body":"@pavlin-policar , thank you so much for your hard work to improve the quality of the code base! It is much appreciated!\r\n\r\nI will be going through your changes in detail, but one thing I wanted to ask about was the CMakeLists.txt. You mentioned you added it because intelliJ suggested it, but I'm a little worried that it complicates things for users. In particular, it requires users to have Cmake installed so that it can generate a makefile. For cross-platform projects with lots of dependencies that are difficult to link, it is worth it, but here the compilation is so simple that I do not think it is necessary.\r\n\r\nDo you think you could remove the CmakeLists (and associated lines in the .gitignore)? Please let me know if I am perhaps missing the point of it..."}}
{"actor":{"display_login":"linqiaozhi"}}