{"action":"created","issue":{"url":"https://api.github.com/repos/broadinstitute/picard/issues/480","repository_url":"https://api.github.com/repos/broadinstitute/picard","labels_url":"https://api.github.com/repos/broadinstitute/picard/issues/480/labels{/name}","comments_url":"https://api.github.com/repos/broadinstitute/picard/issues/480/comments","events_url":"https://api.github.com/repos/broadinstitute/picard/issues/480/events","html_url":"https://github.com/broadinstitute/picard/pull/480","id":139964232,"number":480,"title":"Please review, DON'T MERGE","user":{"login":"SHuang-Broad","id":16310888,"avatar_url":"https://avatars.githubusercontent.com/u/16310888?v=3","gravatar_id":"","url":"https://api.github.com/users/SHuang-Broad","html_url":"https://github.com/SHuang-Broad","followers_url":"https://api.github.com/users/SHuang-Broad/followers","following_url":"https://api.github.com/users/SHuang-Broad/following{/other_user}","gists_url":"https://api.github.com/users/SHuang-Broad/gists{/gist_id}","starred_url":"https://api.github.com/users/SHuang-Broad/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/SHuang-Broad/subscriptions","organizations_url":"https://api.github.com/users/SHuang-Broad/orgs","repos_url":"https://api.github.com/users/SHuang-Broad/repos","events_url":"https://api.github.com/users/SHuang-Broad/events{/privacy}","received_events_url":"https://api.github.com/users/SHuang-Broad/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-03-10T17:54:03Z","updated_at":"2016-03-11T03:06:46Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/broadinstitute/picard/pulls/480","html_url":"https://github.com/broadinstitute/picard/pull/480","diff_url":"https://github.com/broadinstitute/picard/pull/480.diff","patch_url":"https://github.com/broadinstitute/picard/pull/480.patch"},"body":"There are currently several issues in the master branch that causes Picard to generate wrong results on Histogram bin width values when using `CollectInsertSizeMetrics` tool, and since the unit test in `CollectInsertSizeMetricsTest` is not catching it, this subtle bug slipped through the watching eyes.\r\n\r\nSpecifically, this PR addressed the following issues:\r\n1. Added several `MetricAccumulationLevel` values so unit tests now could catch errors (master branch currently only test `MetricAccumulationLevel::ALL_READS`).\r\n2. Corrected several expected values in the unit tests that were from running production Picard on the test bam file (see git diff for which values are changed).\r\n3. The proposed correction on line 152 in `InsertSizeMetricsCollector` now generates \"correct\" bin width values for library \"Solexa-41734\", whose bin width, before the proposed fix, were calculated to be (5,5,5,5,5,0,0,0,0,0), which is apparently problematic, because this sequence should be a (not necessarily strict) monotonically increasing sequence.\r\n\r\n@yfarjoun proposed this minimum-modification correction that will correct the mistake and leave other values unchanged. Alternative proposals on how to fix is to follow.\r\n\r\nThanks.\r\n\r\nPS: some code is commented out for quick fix (R script for generating histogram plot). Python-related code change is mysteriously made by Intellij to work, which I don't quite understand yet. But these don't interfere with the issue raised."},"comment":{"url":"https://api.github.com/repos/broadinstitute/picard/issues/comments/195157402","html_url":"https://github.com/broadinstitute/picard/pull/480#issuecomment-195157402","issue_url":"https://api.github.com/repos/broadinstitute/picard/issues/480","id":195157402,"user":{"login":"yfarjoun","id":2745554,"avatar_url":"https://avatars.githubusercontent.com/u/2745554?v=3","gravatar_id":"","url":"https://api.github.com/users/yfarjoun","html_url":"https://github.com/yfarjoun","followers_url":"https://api.github.com/users/yfarjoun/followers","following_url":"https://api.github.com/users/yfarjoun/following{/other_user}","gists_url":"https://api.github.com/users/yfarjoun/gists{/gist_id}","starred_url":"https://api.github.com/users/yfarjoun/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/yfarjoun/subscriptions","organizations_url":"https://api.github.com/users/yfarjoun/orgs","repos_url":"https://api.github.com/users/yfarjoun/repos","events_url":"https://api.github.com/users/yfarjoun/events{/privacy}","received_events_url":"https://api.github.com/users/yfarjoun/received_events","type":"User","site_admin":false},"created_at":"2016-03-11T03:06:46Z","updated_at":"2016-03-11T03:06:46Z","body":"@SHuang-Broad The removal of the Rcode is confusing as is the inclusion of the .ipr and .iml files. could you please undo those changes (we refrain from including iml and ipr changes since intelliJ is very specific about them and insignificant changes will otherwise get submitted.)\r\n\r\nThat commit (which does the above) should be squashed into the first commit so that the proposal can be evaluated more cleanly (if you're not sure how to do that reordering, let me know I'm happy to assist). I'm glad to see that the proposed change didn't have an effect on the other tests, only on the edge case with 2 points. \r\n\r\nRegarding the change itself, this highlights that in the (rare) case where the median is a half-integer, the algorithm as currently implemented is slightly biased towards slightly low values since the casting will **subtract** 0.5 from both `low` and `high`. My personal view is that `low` and `high` should be initialized (in lines 149-150) to be the floor and the ceil of the median (respectively) this should cause minimal disruption but make the calculation unbiased.\r\n\r\nHappy to hear comments."}}