{"action":"created","issue":{"url":"https://api.github.com/repos/lgatto/MSnbase/issues/72","labels_url":"https://api.github.com/repos/lgatto/MSnbase/issues/72/labels{/name}","comments_url":"https://api.github.com/repos/lgatto/MSnbase/issues/72/comments","events_url":"https://api.github.com/repos/lgatto/MSnbase/issues/72/events","html_url":"https://github.com/lgatto/MSnbase/pull/72","id":122146489,"number":72,"title":"apply functions columnwise by group","user":{"login":"sgibb","id":1828443,"avatar_url":"https://avatars.githubusercontent.com/u/1828443?v=3","gravatar_id":"","url":"https://api.github.com/users/sgibb","html_url":"https://github.com/sgibb","followers_url":"https://api.github.com/users/sgibb/followers","following_url":"https://api.github.com/users/sgibb/following{/other_user}","gists_url":"https://api.github.com/users/sgibb/gists{/gist_id}","starred_url":"https://api.github.com/users/sgibb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sgibb/subscriptions","organizations_url":"https://api.github.com/users/sgibb/orgs","repos_url":"https://api.github.com/users/sgibb/repos","events_url":"https://api.github.com/users/sgibb/events{/privacy}","received_events_url":"https://api.github.com/users/sgibb/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2015-12-14T22:29:00Z","updated_at":"2015-12-15T16:26:35Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/lgatto/MSnbase/pulls/72","html_url":"https://github.com/lgatto/MSnbase/pull/72","diff_url":"https://github.com/lgatto/MSnbase/pull/72.diff","patch_url":"https://github.com/lgatto/MSnbase/pull/72.patch"},"body":"This PR introduces `utils.applyColumnwiseByGroup` a function similar to `tapply` but works on matrices and applies functions like `colSums` groupwise.\r\n\r\nWhile looking into `nQuants` I recognized that it does exactly the same as `featureCV` (except one line => the workhorse function) but with different and complicated code. I removed the duplicated code and create a new function `utils.applyColumnwiseByGroup` instead.\r\n\r\nChanges:\r\n- `featureCV` adapted to `utils.applyColumnwiseByGroup`\r\n- `nQuants` adapted to `utils.applyColumnwiseByGroup`\r\n- unit test for `utils.applyColumnwiseByGroup`\r\n\r\nI also recognized that `nQuants` arguments are completely different from `topN`, `combineFeatures` etc. That's why I replaced the `fcol` argument by the `groupBy` argument that is present in all other `MSnSet` functions. This **breaks the API**! Should I revert it?\r\n\r\nIHMO a lot of more functions could use the `utils.applyColumnwiseByGroup` function. E.g. the `combineMatrixFeatures` could use it instead of its `by(...); do.call(rbind, ...)` approach.\r\nShould I try to convert them as well?\r\n\r\nBTW: I don't like the name `utils.applyColumnwiseByGroup`."},"comment":{"url":"https://api.github.com/repos/lgatto/MSnbase/issues/comments/164817828","html_url":"https://github.com/lgatto/MSnbase/pull/72#issuecomment-164817828","issue_url":"https://api.github.com/repos/lgatto/MSnbase/issues/72","id":164817828,"user":{"login":"sgibb","id":1828443,"avatar_url":"https://avatars.githubusercontent.com/u/1828443?v=3","gravatar_id":"","url":"https://api.github.com/users/sgibb","html_url":"https://github.com/sgibb","followers_url":"https://api.github.com/users/sgibb/followers","following_url":"https://api.github.com/users/sgibb/following{/other_user}","gists_url":"https://api.github.com/users/sgibb/gists{/gist_id}","starred_url":"https://api.github.com/users/sgibb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sgibb/subscriptions","organizations_url":"https://api.github.com/users/sgibb/orgs","repos_url":"https://api.github.com/users/sgibb/repos","events_url":"https://api.github.com/users/sgibb/events{/privacy}","received_events_url":"https://api.github.com/users/sgibb/received_events","type":"User","site_admin":false},"created_at":"2015-12-15T16:26:35Z","updated_at":"2015-12-15T16:26:35Z","body":"> (I agree that this is not consistent, and should probably be addressed).\r\nThat's why I changed it. IMHO there is no reason why `nQuants` should be different from `topN`."}}