{"action":"created","comment":{"url":"https://api.github.com/repos/samtools/htsjdk/pulls/comments/35583823","id":35583823,"diff_hunk":"@@ -84,7 +85,10 @@ public int hashCode() {\n         int result = 0;\n         for (final Field f : getClass().getFields()) {\n             try {\n-                result = 31 * result + f.get(this).hashCode();\n+                Object value = f.get(this);\n+                value = new FormatUtil().format(value); //format the value the way it will be written to disk","path":"src/java/htsjdk/samtools/metrics/MetricBase.java","position":31,"original_position":31,"commit_id":"708bde6264e9ef51f5d7171c74dccad67c4b5ac9","original_commit_id":"bcc1154c10ed0f13584cbc5c650fb7c6d2fc71ef","user":{"login":"lbergelson","id":4700332,"avatar_url":"https://avatars.githubusercontent.com/u/4700332?v=3","gravatar_id":"","url":"https://api.github.com/users/lbergelson","html_url":"https://github.com/lbergelson","followers_url":"https://api.github.com/users/lbergelson/followers","following_url":"https://api.github.com/users/lbergelson/following{/other_user}","gists_url":"https://api.github.com/users/lbergelson/gists{/gist_id}","starred_url":"https://api.github.com/users/lbergelson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lbergelson/subscriptions","organizations_url":"https://api.github.com/users/lbergelson/orgs","repos_url":"https://api.github.com/users/lbergelson/repos","events_url":"https://api.github.com/users/lbergelson/events{/privacy}","received_events_url":"https://api.github.com/users/lbergelson/received_events","type":"User","site_admin":false},"body":"I agree, it doesn't seem ideal, but the alternative possibly more performant way was was pretty gross and brittle. \r\n\r\n\r\nWill do.  Any objection to either \r\n+ creating a default formatter singleton \r\n\r\nor\r\n\r\n+  rewrite FormatUtil to just have static methods, possibly breaking any client code the extends FormatUtils.  (which would cleaner but maybe unpalatable)","created_at":"2015-07-27T20:40:08Z","updated_at":"2015-07-27T20:40:08Z","html_url":"https://github.com/samtools/htsjdk/pull/280#discussion_r35583823","pull_request_url":"https://api.github.com/repos/samtools/htsjdk/pulls/280","_links":{"self":{"href":"https://api.github.com/repos/samtools/htsjdk/pulls/comments/35583823"},"html":{"href":"https://github.com/samtools/htsjdk/pull/280#discussion_r35583823"},"pull_request":{"href":"https://api.github.com/repos/samtools/htsjdk/pulls/280"}}},"pull_request":{"url":"https://api.github.com/repos/samtools/htsjdk/pulls/280","id":40157242,"html_url":"https://github.com/samtools/htsjdk/pull/280","diff_url":"https://github.com/samtools/htsjdk/pull/280.diff","patch_url":"https://github.com/samtools/htsjdk/pull/280.patch","issue_url":"https://api.github.com/repos/samtools/htsjdk/issues/280","number":280,"state":"open","locked":false,"title":"Fix 2 bugs in MetricsBase","user":{"login":"lbergelson","id":4700332,"avatar_url":"https://avatars.githubusercontent.com/u/4700332?v=3","gravatar_id":"","url":"https://api.github.com/users/lbergelson","html_url":"https://github.com/lbergelson","followers_url":"https://api.github.com/users/lbergelson/followers","following_url":"https://api.github.com/users/lbergelson/following{/other_user}","gists_url":"https://api.github.com/users/lbergelson/gists{/gist_id}","starred_url":"https://api.github.com/users/lbergelson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lbergelson/subscriptions","organizations_url":"https://api.github.com/users/lbergelson/orgs","repos_url":"https://api.github.com/users/lbergelson/repos","events_url":"https://api.github.com/users/lbergelson/events{/privacy}","received_events_url":"https://api.github.com/users/lbergelson/received_events","type":"User","site_admin":false},"body":"I split this into 2 commits, the first should be uncontroversial, the second I want to make sure no one objects too.\r\n\r\nCommit 1:\r\n   Fix a bug in the implementation of `hashCode()` in `MetricsBase` it was crashing if any values were `null`.\r\n\r\nCommit 2: \r\n   Fix what I consider a bug in the `equals` method of `MetricBase`\r\n   Currently, the equality is exact equality with respect to floating point numbers.  However, when written to disk a MetricsBase has limited floating point precision, so writing something to disk and reading it back in will sometimes not equal the original file. I've altered the `equality` and `hashCode` methods to compare by using the same representation of floating point numbers as is written to disk.  This is technically backwards compatibility breaking, but I think the current behavior is erroneous.  The downside is that the equality code is complicated now.  \r\n  The simpler and much more guaranteed way to check this would be to actually format the whole thing as an string and then compare string equality.  This would guarantee that equals and hashcode will always be in synch with the on disk representation, but will likely come at a performance cost. \r\n\r\nI've also deleted the `equals` specialization for `MetricsBase`s rather than having 2 complex implementations of equals.  This introduces another backwards incompatible change, since this method made no check for actual class which would allow comparing any `MetricBase` subclass to any other.  However, this equals method was broken.  It was an asymmetric equals method.  It checked the fields in one class against the same fields in the other class, but did not make any checks that those fields were all the fields in the second class.  I.e. \r\n\r\n```\r\n\tpublic class A extends MetricBase {\r\n\t\tpublic int a = 1;\r\n\t}\r\n\tpublic class B extends A{\r\n\t\tpublic int b = 1;\r\n\t}\r\n\r\n\t@Test\r\n\tpublic void testAB(){\r\n\t\tA a = new A();\r\n\t\tB b = new B();\r\n\t\tAssert.assertTrue(a.equals(b));  //true\r\n\t\tAssert.assertTrue(b.equals(a)); // illegal argument exception\r\n\t}\r\n```\r\n\r\nAfter my changes both ways will evaluate to false.  \r\n\r\n@yfarjoun @nh13 @tfenne Let me know what you think.\r\n","created_at":"2015-07-16T17:58:10Z","updated_at":"2015-07-27T20:40:08Z","closed_at":null,"merged_at":null,"merge_commit_sha":"e71b0aa3d6fe5c124f8d2973edd9d6a84768807a","assignee":{"login":"tfenne","id":1609210,"avatar_url":"https://avatars.githubusercontent.com/u/1609210?v=3","gravatar_id":"","url":"https://api.github.com/users/tfenne","html_url":"https://github.com/tfenne","followers_url":"https://api.github.com/users/tfenne/followers","following_url":"https://api.github.com/users/tfenne/following{/other_user}","gists_url":"https://api.github.com/users/tfenne/gists{/gist_id}","starred_url":"https://api.github.com/users/tfenne/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tfenne/subscriptions","organizations_url":"https://api.github.com/users/tfenne/orgs","repos_url":"https://api.github.com/users/tfenne/repos","events_url":"https://api.github.com/users/tfenne/events{/privacy}","received_events_url":"https://api.github.com/users/tfenne/received_events","type":"User","site_admin":false},"milestone":null,"commits_url":"https://api.github.com/repos/samtools/htsjdk/pulls/280/commits","review_comments_url":"https://api.github.com/repos/samtools/htsjdk/pulls/280/comments","review_comment_url":"https://api.github.com/repos/samtools/htsjdk/pulls/comments{/number}","comments_url":"https://api.github.com/repos/samtools/htsjdk/issues/280/comments","statuses_url":"https://api.github.com/repos/samtools/htsjdk/statuses/708bde6264e9ef51f5d7171c74dccad67c4b5ac9","head":{"label":"samtools:lb_Metrics_BugFixes","ref":"lb_Metrics_BugFixes","sha":"708bde6264e9ef51f5d7171c74dccad67c4b5ac9","user":{"login":"samtools","id":1518450,"avatar_url":"https://avatars.githubusercontent.com/u/1518450?v=3","gravatar_id":"","url":"https://api.github.com/users/samtools","html_url":"https://github.com/samtools","followers_url":"https://api.github.com/users/samtools/followers","following_url":"https://api.github.com/users/samtools/following{/other_user}","gists_url":"https://api.github.com/users/samtools/gists{/gist_id}","starred_url":"https://api.github.com/users/samtools/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/samtools/subscriptions","organizations_url":"https://api.github.com/users/samtools/orgs","repos_url":"https://api.github.com/users/samtools/repos","events_url":"https://api.github.com/users/samtools/events{/privacy}","received_events_url":"https://api.github.com/users/samtools/received_events","type":"Organization","site_admin":false},"repo":{"id":19272315,"name":"htsjdk","full_name":"samtools/htsjdk","owner":{"login":"samtools","id":1518450,"avatar_url":"https://avatars.githubusercontent.com/u/1518450?v=3","gravatar_id":"","url":"https://api.github.com/users/samtools","html_url":"https://github.com/samtools","followers_url":"https://api.github.com/users/samtools/followers","following_url":"https://api.github.com/users/samtools/following{/other_user}","gists_url":"https://api.github.com/users/samtools/gists{/gist_id}","starred_url":"https://api.github.com/users/samtools/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/samtools/subscriptions","organizations_url":"https://api.github.com/users/samtools/orgs","repos_url":"https://api.github.com/users/samtools/repos","events_url":"https://api.github.com/users/samtools/events{/privacy}","received_events_url":"https://api.github.com/users/samtools/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/samtools/htsjdk","description":"A Java API for high-throughput sequencing data (HTS) formats.","fork":false,"url":"https://api.github.com/repos/samtools/htsjdk","forks_url":"https://api.github.com/repos/samtools/htsjdk/forks","keys_url":"https://api.github.com/repos/samtools/htsjdk/keys{/key_id}","collaborators_url":"https://api.github.com/repos/samtools/htsjdk/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/samtools/htsjdk/teams","hooks_url":"https://api.github.com/repos/samtools/htsjdk/hooks","issue_events_url":"https://api.github.com/repos/samtools/htsjdk/issues/events{/number}","events_url":"https://api.github.com/repos/samtools/htsjdk/events","assignees_url":"https://api.github.com/repos/samtools/htsjdk/assignees{/user}","branches_url":"https://api.github.com/repos/samtools/htsjdk/branches{/branch}","tags_url":"https://api.github.com/repos/samtools/htsjdk/tags","blobs_url":"https://api.github.com/repos/samtools/htsjdk/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/samtools/htsjdk/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/samtools/htsjdk/git/refs{/sha}","trees_url":"https://api.github.com/repos/samtools/htsjdk/git/trees{/sha}","statuses_url":"https://api.github.com/repos/samtools/htsjdk/statuses/{sha}","languages_url":"https://api.github.com/repos/samtools/htsjdk/languages","stargazers_url":"https://api.github.com/repos/samtools/htsjdk/stargazers","contributors_url":"https://api.github.com/repos/samtools/htsjdk/contributors","subscribers_url":"https://api.github.com/repos/samtools/htsjdk/subscribers","subscription_url":"https://api.github.com/repos/samtools/htsjdk/subscription","commits_url":"https://api.github.com/repos/samtools/htsjdk/commits{/sha}","git_commits_url":"https://api.github.com/repos/samtools/htsjdk/git/commits{/sha}","comments_url":"https://api.github.com/repos/samtools/htsjdk/comments{/number}","issue_comment_url":"https://api.github.com/repos/samtools/htsjdk/issues/comments{/number}","contents_url":"https://api.github.com/repos/samtools/htsjdk/contents/{+path}","compare_url":"https://api.github.com/repos/samtools/htsjdk/compare/{base}...{head}","merges_url":"https://api.github.com/repos/samtools/htsjdk/merges","archive_url":"https://api.github.com/repos/samtools/htsjdk/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/samtools/htsjdk/downloads","issues_url":"https://api.github.com/repos/samtools/htsjdk/issues{/number}","pulls_url":"https://api.github.com/repos/samtools/htsjdk/pulls{/number}","milestones_url":"https://api.github.com/repos/samtools/htsjdk/milestones{/number}","notifications_url":"https://api.github.com/repos/samtools/htsjdk/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/samtools/htsjdk/labels{/name}","releases_url":"https://api.github.com/repos/samtools/htsjdk/releases{/id}","created_at":"2014-04-29T10:11:28Z","updated_at":"2015-07-27T19:15:11Z","pushed_at":"2015-07-27T19:11:01Z","git_url":"git://github.com/samtools/htsjdk.git","ssh_url":"git@github.com:samtools/htsjdk.git","clone_url":"https://github.com/samtools/htsjdk.git","svn_url":"https://github.com/samtools/htsjdk","homepage":"http://samtools.github.io/htsjdk/","size":63600,"stargazers_count":50,"watchers_count":50,"language":"Java","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":true,"forks_count":63,"mirror_url":null,"open_issues_count":35,"forks":63,"open_issues":35,"watchers":50,"default_branch":"master"}},"base":{"label":"samtools:master","ref":"master","sha":"b2638e03bf07ea66dd73064f7e04189f0b6338bd","user":{"login":"samtools","id":1518450,"avatar_url":"https://avatars.githubusercontent.com/u/1518450?v=3","gravatar_id":"","url":"https://api.github.com/users/samtools","html_url":"https://github.com/samtools","followers_url":"https://api.github.com/users/samtools/followers","following_url":"https://api.github.com/users/samtools/following{/other_user}","gists_url":"https://api.github.com/users/samtools/gists{/gist_id}","starred_url":"https://api.github.com/users/samtools/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/samtools/subscriptions","organizations_url":"https://api.github.com/users/samtools/orgs","repos_url":"https://api.github.com/users/samtools/repos","events_url":"https://api.github.com/users/samtools/events{/privacy}","received_events_url":"https://api.github.com/users/samtools/received_events","type":"Organization","site_admin":false},"repo":{"id":19272315,"name":"htsjdk","full_name":"samtools/htsjdk","owner":{"login":"samtools","id":1518450,"avatar_url":"https://avatars.githubusercontent.com/u/1518450?v=3","gravatar_id":"","url":"https://api.github.com/users/samtools","html_url":"https://github.com/samtools","followers_url":"https://api.github.com/users/samtools/followers","following_url":"https://api.github.com/users/samtools/following{/other_user}","gists_url":"https://api.github.com/users/samtools/gists{/gist_id}","starred_url":"https://api.github.com/users/samtools/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/samtools/subscriptions","organizations_url":"https://api.github.com/users/samtools/orgs","repos_url":"https://api.github.com/users/samtools/repos","events_url":"https://api.github.com/users/samtools/events{/privacy}","received_events_url":"https://api.github.com/users/samtools/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/samtools/htsjdk","description":"A Java API for high-throughput sequencing data (HTS) formats.","fork":false,"url":"https://api.github.com/repos/samtools/htsjdk","forks_url":"https://api.github.com/repos/samtools/htsjdk/forks","keys_url":"https://api.github.com/repos/samtools/htsjdk/keys{/key_id}","collaborators_url":"https://api.github.com/repos/samtools/htsjdk/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/samtools/htsjdk/teams","hooks_url":"https://api.github.com/repos/samtools/htsjdk/hooks","issue_events_url":"https://api.github.com/repos/samtools/htsjdk/issues/events{/number}","events_url":"https://api.github.com/repos/samtools/htsjdk/events","assignees_url":"https://api.github.com/repos/samtools/htsjdk/assignees{/user}","branches_url":"https://api.github.com/repos/samtools/htsjdk/branches{/branch}","tags_url":"https://api.github.com/repos/samtools/htsjdk/tags","blobs_url":"https://api.github.com/repos/samtools/htsjdk/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/samtools/htsjdk/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/samtools/htsjdk/git/refs{/sha}","trees_url":"https://api.github.com/repos/samtools/htsjdk/git/trees{/sha}","statuses_url":"https://api.github.com/repos/samtools/htsjdk/statuses/{sha}","languages_url":"https://api.github.com/repos/samtools/htsjdk/languages","stargazers_url":"https://api.github.com/repos/samtools/htsjdk/stargazers","contributors_url":"https://api.github.com/repos/samtools/htsjdk/contributors","subscribers_url":"https://api.github.com/repos/samtools/htsjdk/subscribers","subscription_url":"https://api.github.com/repos/samtools/htsjdk/subscription","commits_url":"https://api.github.com/repos/samtools/htsjdk/commits{/sha}","git_commits_url":"https://api.github.com/repos/samtools/htsjdk/git/commits{/sha}","comments_url":"https://api.github.com/repos/samtools/htsjdk/comments{/number}","issue_comment_url":"https://api.github.com/repos/samtools/htsjdk/issues/comments{/number}","contents_url":"https://api.github.com/repos/samtools/htsjdk/contents/{+path}","compare_url":"https://api.github.com/repos/samtools/htsjdk/compare/{base}...{head}","merges_url":"https://api.github.com/repos/samtools/htsjdk/merges","archive_url":"https://api.github.com/repos/samtools/htsjdk/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/samtools/htsjdk/downloads","issues_url":"https://api.github.com/repos/samtools/htsjdk/issues{/number}","pulls_url":"https://api.github.com/repos/samtools/htsjdk/pulls{/number}","milestones_url":"https://api.github.com/repos/samtools/htsjdk/milestones{/number}","notifications_url":"https://api.github.com/repos/samtools/htsjdk/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/samtools/htsjdk/labels{/name}","releases_url":"https://api.github.com/repos/samtools/htsjdk/releases{/id}","created_at":"2014-04-29T10:11:28Z","updated_at":"2015-07-27T19:15:11Z","pushed_at":"2015-07-27T19:11:01Z","git_url":"git://github.com/samtools/htsjdk.git","ssh_url":"git@github.com:samtools/htsjdk.git","clone_url":"https://github.com/samtools/htsjdk.git","svn_url":"https://github.com/samtools/htsjdk","homepage":"http://samtools.github.io/htsjdk/","size":63600,"stargazers_count":50,"watchers_count":50,"language":"Java","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":true,"forks_count":63,"mirror_url":null,"open_issues_count":35,"forks":63,"open_issues":35,"watchers":50,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/samtools/htsjdk/pulls/280"},"html":{"href":"https://github.com/samtools/htsjdk/pull/280"},"issue":{"href":"https://api.github.com/repos/samtools/htsjdk/issues/280"},"comments":{"href":"https://api.github.com/repos/samtools/htsjdk/issues/280/comments"},"review_comments":{"href":"https://api.github.com/repos/samtools/htsjdk/pulls/280/comments"},"review_comment":{"href":"https://api.github.com/repos/samtools/htsjdk/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/samtools/htsjdk/pulls/280/commits"},"statuses":{"href":"https://api.github.com/repos/samtools/htsjdk/statuses/708bde6264e9ef51f5d7171c74dccad67c4b5ac9"}}}}