{"action":"created","issue":{"url":"https://api.github.com/repos/pyoceans/python-oceans/issues/7","labels_url":"https://api.github.com/repos/pyoceans/python-oceans/issues/7/labels{/name}","comments_url":"https://api.github.com/repos/pyoceans/python-oceans/issues/7/comments","events_url":"https://api.github.com/repos/pyoceans/python-oceans/issues/7/events","html_url":"https://github.com/pyoceans/python-oceans/issues/7","id":51936490,"number":7,"title":"@efiring's review and suggestions for `gamma_GP_from_SP_pt.py`","user":{"login":"ocefpaf","id":950575,"avatar_url":"https://avatars.githubusercontent.com/u/950575?v=3","gravatar_id":"","url":"https://api.github.com/users/ocefpaf","html_url":"https://github.com/ocefpaf","followers_url":"https://api.github.com/users/ocefpaf/followers","following_url":"https://api.github.com/users/ocefpaf/following{/other_user}","gists_url":"https://api.github.com/users/ocefpaf/gists{/gist_id}","starred_url":"https://api.github.com/users/ocefpaf/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ocefpaf/subscriptions","organizations_url":"https://api.github.com/users/ocefpaf/orgs","repos_url":"https://api.github.com/users/ocefpaf/repos","events_url":"https://api.github.com/users/ocefpaf/events{/privacy}","received_events_url":"https://api.github.com/users/ocefpaf/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2014-12-14T23:33:04Z","updated_at":"2015-03-03T20:14:53Z","closed_at":null,"body":"> (Trivial first aesthetic tweak: use a list comprehension or generator in the first line.  But maybe all this would be handled in a decorator eventually anyway.)\r\n\r\n> In this case, the simplest change I would make is to leave the coefficient tables as lists of lists, or lists of tuples.  Converting to ndarrays is not helping anything, and is actually adding a tiny bit of overhead.\r\n\r\n> The loops could use the more idiomatic form:\r\n```python\r\ngamma = np.zeros_like(SP)\r\nfor i, j, c in Fit:\r\n    gamma += c * (SP**i * pt**j)\r\n```\r\n\r\n> Going farther, I would take all the Fit tables out of the functions, make them named module-level variables, and probably collect them in a dictionary. (Same for the polygons, which I would probably save as 2-D ndarrays--I think shapely can handle them efficiently.) Then I would use a single function to handle the loop, with the Fit table as a third argument.  This would allow for future optimization of that single critical function, if desired; it would clarify the common logic; it would minimize repetition; and it would be slightly faster because all the table initialization would be done only once, at import time.\r\n\r\n> Additionally, I would look at the main function to see whether it is really necessary to calculate gamma for all the oceans; I suspect one could calculate weights first, and omit the gamma calculation for any region with zero weight.\r\n\r\n> Is some normalization of the input lat/lon range needed to make everything work with the region polygons?  Are there potential problems with crossing the dateline, or the prime meridian?"},"comment":{"url":"https://api.github.com/repos/pyoceans/python-oceans/issues/comments/77025121","html_url":"https://github.com/pyoceans/python-oceans/issues/7#issuecomment-77025121","issue_url":"https://api.github.com/repos/pyoceans/python-oceans/issues/7","id":77025121,"user":{"login":"ocefpaf","id":950575,"avatar_url":"https://avatars.githubusercontent.com/u/950575?v=3","gravatar_id":"","url":"https://api.github.com/users/ocefpaf","html_url":"https://github.com/ocefpaf","followers_url":"https://api.github.com/users/ocefpaf/followers","following_url":"https://api.github.com/users/ocefpaf/following{/other_user}","gists_url":"https://api.github.com/users/ocefpaf/gists{/gist_id}","starred_url":"https://api.github.com/users/ocefpaf/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ocefpaf/subscriptions","organizations_url":"https://api.github.com/users/ocefpaf/orgs","repos_url":"https://api.github.com/users/ocefpaf/repos","events_url":"https://api.github.com/users/ocefpaf/events{/privacy}","received_events_url":"https://api.github.com/users/ocefpaf/received_events","type":"User","site_admin":false},"created_at":"2015-03-03T20:14:53Z","updated_at":"2015-03-03T20:14:53Z","body":"This function is not publicly listed because it is experimental and needs more testing.  That way we force people using it to know where they are importing from.\r\n\r\nBTW:  Lets keep this issue about the review actions.  In cases like this open a new issue."}}