{"action":"created","issue":{"url":"https://api.github.com/repos/criteo/biggraphite/issues/35","repository_url":"https://api.github.com/repos/criteo/biggraphite","labels_url":"https://api.github.com/repos/criteo/biggraphite/issues/35/labels{/name}","comments_url":"https://api.github.com/repos/criteo/biggraphite/issues/35/comments","events_url":"https://api.github.com/repos/criteo/biggraphite/issues/35/events","html_url":"https://github.com/criteo/biggraphite/pull/35","id":159907077,"number":35,"title":"Add a script in utils/ to compute diff between two graphite clusters","user":{"login":"Thib17","id":13062201,"avatar_url":"https://avatars.githubusercontent.com/u/13062201?v=3","gravatar_id":"","url":"https://api.github.com/users/Thib17","html_url":"https://github.com/Thib17","followers_url":"https://api.github.com/users/Thib17/followers","following_url":"https://api.github.com/users/Thib17/following{/other_user}","gists_url":"https://api.github.com/users/Thib17/gists{/gist_id}","starred_url":"https://api.github.com/users/Thib17/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Thib17/subscriptions","organizations_url":"https://api.github.com/users/Thib17/orgs","repos_url":"https://api.github.com/users/Thib17/repos","events_url":"https://api.github.com/users/Thib17/events{/privacy}","received_events_url":"https://api.github.com/users/Thib17/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2016-06-13T09:24:34Z","updated_at":"2016-06-15T13:08:05Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/criteo/biggraphite/pulls/35","html_url":"https://github.com/criteo/biggraphite/pull/35","diff_url":"https://github.com/criteo/biggraphite/pull/35.diff","patch_url":"https://github.com/criteo/biggraphite/pull/35.patch"},"body":"There is no test because the script is not critical and not already stable.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"35\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/criteo/biggraphite/35)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/criteo/biggraphite/issues/comments/226181438","html_url":"https://github.com/criteo/biggraphite/pull/35#issuecomment-226181438","issue_url":"https://api.github.com/repos/criteo/biggraphite/issues/35","id":226181438,"user":{"login":"unbrice","id":530291,"avatar_url":"https://avatars.githubusercontent.com/u/530291?v=3","gravatar_id":"","url":"https://api.github.com/users/unbrice","html_url":"https://github.com/unbrice","followers_url":"https://api.github.com/users/unbrice/followers","following_url":"https://api.github.com/users/unbrice/following{/other_user}","gists_url":"https://api.github.com/users/unbrice/gists{/gist_id}","starred_url":"https://api.github.com/users/unbrice/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/unbrice/subscriptions","organizations_url":"https://api.github.com/users/unbrice/orgs","repos_url":"https://api.github.com/users/unbrice/repos","events_url":"https://api.github.com/users/unbrice/events{/privacy}","received_events_url":"https://api.github.com/users/unbrice/received_events","type":"User","site_admin":false},"created_at":"2016-06-15T13:08:05Z","updated_at":"2016-06-15T13:08:05Z","body":"\n\n\n\nReview status: 0 of 1 files reviewed at latest revision, 21 unresolved discussions, some commit checks broke.\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 19 \\[r6\\]](https://reviewable.io:443/reviews/criteo/biggraphite/35#-KKJ8t-SCpINJUe4JSF_:-KKJ8t-SCpINJUe4JSFa:9518698) ([raw file](https://github.com/criteo/biggraphite/blob/a79c6c12f25b963d09ef6f6bf1361fec54c8d460/biggraphite/cli/clusters_diff.py#L19)):*\n> ```Python\n> _PRINTER = None\n> \n> Fetched_Query = collections.namedtuple(\n> ```\n\nUpperCaseForClasses\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 148 \\[r6\\]](https://reviewable.io:443/reviews/criteo/biggraphite/35#-KKJ9tCFH6dv2pvyn1o8:-KKJ9tCFH6dv2pvyn1o9:-2064214023) ([raw file](https://github.com/criteo/biggraphite/blob/a79c6c12f25b963d09ef6f6bf1361fec54c8d460/biggraphite/cli/clusters_diff.py#L148)):*\n> ```Python\n> \n>     def _get_avg_relative_distance_per_point(self, first_datapoints_dict, second_datapoints_dict):\n>         common_timestamps = set(first_datapoints_dict).intersection(set(second_datapoints_dict))\n> ```\n\nfirst_datapoints_dict.viewkeys()&second_datapoints_dict.viewkeys()\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 152 \\[r6\\]](https://reviewable.io:443/reviews/criteo/biggraphite/35#-KKJ9hqfRTogi8wORQAW:-KKJ9hqfRTogi8wORQAX:-942592800) ([raw file](https://github.com/criteo/biggraphite/blob/a79c6c12f25b963d09ef6f6bf1361fec54c8d460/biggraphite/cli/clusters_diff.py#L152)):*\n> ```Python\n>         total_number_timestamps = (\n>             len(set(first_datapoints_dict).union(set(second_datapoints_dict))))\n>         sum_relative_distance = (total_number_timestamps-len(common_timestamps)) * 100.0\n> ```\n\nrather than multiplying everything by 100, did you consider just returning a float between 0 and 1, or doing the multiplication just before returning?\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 156 \\[r6\\]](https://reviewable.io:443/reviews/criteo/biggraphite/35#-KKJ9EIa8RF9fhYyzfGU:-KKJ9EIa8RF9fhYyzfGV:-467539127) ([raw file](https://github.com/criteo/biggraphite/blob/a79c6c12f25b963d09ef6f6bf1361fec54c8d460/biggraphite/cli/clusters_diff.py#L156)):*\n> ```Python\n>         # If total_number_timestamps == 0, then neither of the hosts store data for the target,\n>         # so all distances are 0\n>         if (total_number_timestamps == 0):\n> ```\n\nunneeded parenthesis\n\n---\n\n*[biggraphite/cli/clusters_diff.py, line 160 \\[r6\\]](https://reviewable.io:443/reviews/criteo/biggraphite/35#-KKJ9QiM8mM1xAgjmm6H:-KKJ9QiM8mM1xAgjmm6I:1201728396) ([raw file](https://github.com/criteo/biggraphite/blob/a79c6c12f25b963d09ef6f6bf1361fec54c8d460/biggraphite/cli/clusters_diff.py#L160)):*\n> ```Python\n> \n>         for timestamp in common_timestamps:\n>             # If both datapoints_dict store a null, values are equal\n> ```\n\nthis is different: you say that 0 and None are the same. is that intended?\n\n---\n\n*[utils/clusters-diff.py, line 1 \\[r1\\]](https://reviewable.io:443/reviews/criteo/biggraphite/35#-KK8nE2D3GnnhoOnlMAY:-KK8nE2D3GnnhoOnlMAZ:-244045035) ([raw file](https://github.com/criteo/biggraphite/blob/5e81a7fda82a52dcca33e05fda37f6466ded0538/utils/clusters-diff.py#L1)):*\n> ```Python\n> \"\"\"Compare two Graphite clusters for a given list of queries.\"\"\"\n> ```\n\nplease move to biggraphite/cli and run tox\n\n---\n\n*[utils/clusters-diff.py, line 9 \\[r1\\]](https://reviewable.io:443/reviews/criteo/biggraphite/35#-KK8nmmJ50vhOQ6TFYdx:-KK8nmmJ50vhOQ6TFYdy:-1051369168) ([raw file](https://github.com/criteo/biggraphite/blob/5e81a7fda82a52dcca33e05fda37f6466ded0538/utils/clusters-diff.py#L9)):*\n> ```Python\n> from datadiff import diff\n> import numpy as np\n> from sklearn.metrics import jaccard_similarity_score\n> ```\n\nnot used?\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/criteo/biggraphite/35)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"unbrice"}}