{"action":"created","issue":{"url":"https://api.github.com/repos/ericmjl/nxviz/issues/6","repository_url":"https://api.github.com/repos/ericmjl/nxviz","labels_url":"https://api.github.com/repos/ericmjl/nxviz/issues/6/labels{/name}","comments_url":"https://api.github.com/repos/ericmjl/nxviz/issues/6/comments","events_url":"https://api.github.com/repos/ericmjl/nxviz/issues/6/events","html_url":"https://github.com/ericmjl/nxviz/issues/6","id":168436465,"number":6,"title":"Uniform types in set_nodecolors","user":{"login":"leotrs","id":1096704,"avatar_url":"https://avatars.githubusercontent.com/u/1096704?v=3","gravatar_id":"","url":"https://api.github.com/users/leotrs","html_url":"https://github.com/leotrs","followers_url":"https://api.github.com/users/leotrs/followers","following_url":"https://api.github.com/users/leotrs/following{/other_user}","gists_url":"https://api.github.com/users/leotrs/gists{/gist_id}","starred_url":"https://api.github.com/users/leotrs/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/leotrs/subscriptions","organizations_url":"https://api.github.com/users/leotrs/orgs","repos_url":"https://api.github.com/users/leotrs/repos","events_url":"https://api.github.com/users/leotrs/events{/privacy}","received_events_url":"https://api.github.com/users/leotrs/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2016-07-30T00:06:39Z","updated_at":"2016-08-14T23:06:29Z","closed_at":null,"body":"Here is an excerpt the docstring provided for `BasePlot.set_nodecolors`:\r\n```\r\n\"\"\"\r\nIf `nodecolors` is a `string`, all nodes will carry that color.\r\nIf `nodecolors` is a `list` or `tuple`, then nodes will be coloured in\r\norder by the list or tuple elements.\r\nIf `nodecolors` is a `dict`, then the keys have to be all present in\r\nthe nodelist.\r\nBy default, node color is blue.\r\n\"\"\"\r\n```\r\n\r\nThen, the code is filled with `asserts` according to the mentioned rules.\r\n\r\nThis approach is fine, but I'm not a huge fan of overloading a function and then forking its behavior based on its arguments. I have some experience with *Mathematica*, where this practice is used to the extent that built-in functions change behavior not only on the type of their arguments but also on their shape. Debugging them is not a very fun exercise.\r\n\r\n----------------\r\n\r\nHere is what hypothetical calls to `set_nodecolors` would look like:\r\n\r\n```python\r\nplot = BasePlot(nodes=range(6))\r\n\r\n# string: all the same\r\nplot.set_nodecolors('blue')\r\n\r\n# iterable: must preserve order\r\nplot.set_nodecolors(['blue', 'blue', 'blue', 'blue', 'black', 'blue'])\r\n\r\n# dict: control over each value\r\nplot.set_nodecolors({0: 'blue', 1: 'blue', 2: 'blue',\r\n                     3: 'blue', 4: 'black', 5: 'blue'})\r\n\r\n```\r\n\r\nWhat I propose is to homogenize the calls and always using dicts:\r\n\r\n```python\r\nnodes = range(6)\r\nplot = BasePlot(nodes)\r\n\r\n# dict: setting all nodes to the same value is still easy\r\nplot.set_nodecolors({node: 'blue' for node in nodes})\r\n\r\n# no iterables!\r\n# user doesn't worry about correct ordering\r\n\r\n# dict: control over each value\r\nplot.set_nodecolors({0: 'blue', 1: 'blue', 2: 'blue',\r\n                     3: 'blue', 4: 'black', 5: 'blue'})\r\n\r\n```\r\n\r\nThe main benefits of this approach is that the code is much shorter and less error-prone, as there are far fewer type-checks and forks that need to be performed at each call (do we only check the type of the argument, or also the length? What about the validity of each color string?) Also, the user doesn't need to worry about the order of iterables being the same as the nodecolors object they are passing.\r\n\r\nRight now, even if only one string is being passed, the method ultimately performs `self.nodecolors = [nodecolors] * len(self.nodes)`, which means passing a dict of size equal to the nodelist even in the simplest case will not incur in more memory cost.\r\n\r\nI know everything is yet in early stages, but I thought standardizing function signatures is an important design decision to be made from the start.\r\n"},"comment":{"url":"https://api.github.com/repos/ericmjl/nxviz/issues/comments/239703472","html_url":"https://github.com/ericmjl/nxviz/issues/6#issuecomment-239703472","issue_url":"https://api.github.com/repos/ericmjl/nxviz/issues/6","id":239703472,"user":{"login":"ericmjl","id":2631566,"avatar_url":"https://avatars.githubusercontent.com/u/2631566?v=3","gravatar_id":"","url":"https://api.github.com/users/ericmjl","html_url":"https://github.com/ericmjl","followers_url":"https://api.github.com/users/ericmjl/followers","following_url":"https://api.github.com/users/ericmjl/following{/other_user}","gists_url":"https://api.github.com/users/ericmjl/gists{/gist_id}","starred_url":"https://api.github.com/users/ericmjl/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ericmjl/subscriptions","organizations_url":"https://api.github.com/users/ericmjl/orgs","repos_url":"https://api.github.com/users/ericmjl/repos","events_url":"https://api.github.com/users/ericmjl/events{/privacy}","received_events_url":"https://api.github.com/users/ericmjl/received_events","type":"User","site_admin":false},"created_at":"2016-08-14T23:06:29Z","updated_at":"2016-08-14T23:06:29Z","body":"@jonchar is our resident matplotlib guru - what's your thoughts? "}}
{"actor":{"display_login":"ericmjl"}}