{"action":"created","issue":{"url":"https://api.github.com/repos/nipy/dipy/issues/1223","repository_url":"https://api.github.com/repos/nipy/dipy","labels_url":"https://api.github.com/repos/nipy/dipy/issues/1223/labels{/name}","comments_url":"https://api.github.com/repos/nipy/dipy/issues/1223/comments","events_url":"https://api.github.com/repos/nipy/dipy/issues/1223/events","html_url":"https://github.com/nipy/dipy/pull/1223","id":221903756,"number":1223,"title":"local PCA using SVD","user":{"login":"arokem","id":118582,"avatar_url":"https://avatars2.githubusercontent.com/u/118582?v=3","gravatar_id":"","url":"https://api.github.com/users/arokem","html_url":"https://github.com/arokem","followers_url":"https://api.github.com/users/arokem/followers","following_url":"https://api.github.com/users/arokem/following{/other_user}","gists_url":"https://api.github.com/users/arokem/gists{/gist_id}","starred_url":"https://api.github.com/users/arokem/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/arokem/subscriptions","organizations_url":"https://api.github.com/users/arokem/orgs","repos_url":"https://api.github.com/users/arokem/repos","events_url":"https://api.github.com/users/arokem/events{/privacy}","received_events_url":"https://api.github.com/users/arokem/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":59,"created_at":"2017-04-14T22:32:23Z","updated_at":"2017-05-19T11:55:32Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/nipy/dipy/pulls/1223","html_url":"https://github.com/nipy/dipy/pull/1223","diff_url":"https://github.com/nipy/dipy/pull/1223.diff","patch_url":"https://github.com/nipy/dipy/pull/1223.patch"},"body":"This extends #1108 and supersedes it, addressing some of the issue raised in the review of that previous PR.\r\n\r\n1. Following a comment by @samuelstjean (https://github.com/nipy/dipy/pull/1108/files#r81179260), I implemented the PCA using the SVD, instead of decomposing the covariance matrix. This is slower by about 50%, when done using scipy.linalg.svd, but as suggested there, using the direct lapack API performs as fast as the covariance matrix decomposition approach. I am worried that this will not work with older versions of scipy, but let's see what Travis says.\r\n\r\n2. I fixed a bunch of other things, typos, structure of the code overall (the whole thing was wrapped in a bit `if...else` structure for error handling, which I moved to the top).\r\n"},"comment":{"url":"https://api.github.com/repos/nipy/dipy/issues/comments/302683539","html_url":"https://github.com/nipy/dipy/pull/1223#issuecomment-302683539","issue_url":"https://api.github.com/repos/nipy/dipy/issues/1223","id":302683539,"user":{"login":"samuelstjean","id":3030760,"avatar_url":"https://avatars3.githubusercontent.com/u/3030760?v=3","gravatar_id":"","url":"https://api.github.com/users/samuelstjean","html_url":"https://github.com/samuelstjean","followers_url":"https://api.github.com/users/samuelstjean/followers","following_url":"https://api.github.com/users/samuelstjean/following{/other_user}","gists_url":"https://api.github.com/users/samuelstjean/gists{/gist_id}","starred_url":"https://api.github.com/users/samuelstjean/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/samuelstjean/subscriptions","organizations_url":"https://api.github.com/users/samuelstjean/orgs","repos_url":"https://api.github.com/users/samuelstjean/repos","events_url":"https://api.github.com/users/samuelstjean/events{/privacy}","received_events_url":"https://api.github.com/users/samuelstjean/received_events","type":"User","site_admin":false},"created_at":"2017-05-19T11:55:31Z","updated_at":"2017-05-19T11:55:31Z","body":"On quite the contrary, I'd say it definitely does not work. Can't attach\npictures to email and still can't remember my password, but anyway, for the\ndata I mentioned, there is a discrepancy with the original. See the\nattached png screenshot in the dropbox link I posted earlier, it's the\nresiduals for a random volume and the b0 and a comparison of the first\nvolume between both implementation.\n\nIt still should be easier to find the problem and fix it now instead of\nhaving people complaining two years down the line about different behavior\nbetween the original and the reimplementation (true story, that's about how\nlong it took to find out why the CSD was having ringing artefact, turns out\nthe positivity constraints were left out by mistake but the cholesky\ndecomposition enforced them on the side).\n\n2017-05-19 10:36 GMT+02:00 Rafael Neto Henriques <notifications@github.com>:\n\n> Hello guys! From the figures that you are sending me, the algorithm seems\n> to be already working quite well. From the residual of the Ferret's data\n> (particularly for the different diffusion gradient directions), it seems\n> that some structural information is being lost (and that is why denoising\n> data can also look a bit smooth). However, this should be due to the noise\n> sigma estimate. Since here we are using Manjon's empirical signal principal\n> component classification, it is normal that this will work better for some\n> datasets. I think the only way to overcome this is using more robust\n> component classification as proposed by Veraart et al., (2016)\n> <http://www.sciencedirect.com/science/article/pii/S1053811916303949>.\n> This will adapt better for different datasets. I have already implemented\n> this approach in the past, I was just waiting to have this merged so that I\n> can add this in the top of the work done here.\n>\n> This PR seems to contain all what Manjón et al., 2013\n> <http://journals.plos.org/plosone/article?id=10.1371/journal.pone.0073021>\n> proposed. In my point of view, this is ready to be merged!\n>\n> —\n> You are receiving this because you were mentioned.\n> Reply to this email directly, view it on GitHub\n> <https://github.com/nipy/dipy/pull/1223#issuecomment-302643744>, or mute\n> the thread\n> <https://github.com/notifications/unsubscribe-auth/AC4-6NLqScSiAtxEWrUHz2hF2AtKDjhMks5r7VRzgaJpZM4M-JlZ>\n> .\n>\n"}}
{"actor":{"display_login":"samuelstjean"}}