{"action":"created","issue":{"url":"https://api.github.com/repos/astropy/astropy/issues/7293","repository_url":"https://api.github.com/repos/astropy/astropy","labels_url":"https://api.github.com/repos/astropy/astropy/issues/7293/labels{/name}","comments_url":"https://api.github.com/repos/astropy/astropy/issues/7293/comments","events_url":"https://api.github.com/repos/astropy/astropy/issues/7293/events","html_url":"https://github.com/astropy/astropy/pull/7293","id":305798046,"node_id":"MDExOlB1bGxSZXF1ZXN0MTc1NDM3MzEx","number":7293,"title":"Performance enhancements for convolution.convolve","user":{"login":"jamienoss","id":5013975,"node_id":"MDQ6VXNlcjUwMTM5NzU=","avatar_url":"https://avatars2.githubusercontent.com/u/5013975?v=4","gravatar_id":"","url":"https://api.github.com/users/jamienoss","html_url":"https://github.com/jamienoss","followers_url":"https://api.github.com/users/jamienoss/followers","following_url":"https://api.github.com/users/jamienoss/following{/other_user}","gists_url":"https://api.github.com/users/jamienoss/gists{/gist_id}","starred_url":"https://api.github.com/users/jamienoss/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jamienoss/subscriptions","organizations_url":"https://api.github.com/users/jamienoss/orgs","repos_url":"https://api.github.com/users/jamienoss/repos","events_url":"https://api.github.com/users/jamienoss/events{/privacy}","received_events_url":"https://api.github.com/users/jamienoss/received_events","type":"User","site_admin":false},"labels":[{"id":133220484,"node_id":"MDU6TGFiZWwxMzMyMjA0ODQ=","url":"https://api.github.com/repos/astropy/astropy/labels/Performance","name":"Performance","color":"bfe5bf","default":false},{"id":57200118,"node_id":"MDU6TGFiZWw1NzIwMDExOA==","url":"https://api.github.com/repos/astropy/astropy/labels/convolution","name":"convolution","color":"c7def8","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":{"url":"https://api.github.com/repos/astropy/astropy/milestones/52","html_url":"https://github.com/astropy/astropy/milestone/52","labels_url":"https://api.github.com/repos/astropy/astropy/milestones/52/labels","id":2929745,"node_id":"MDk6TWlsZXN0b25lMjkyOTc0NQ==","number":52,"title":"v3.1","description":"","creator":{"login":"eteq","id":346587,"node_id":"MDQ6VXNlcjM0NjU4Nw==","avatar_url":"https://avatars1.githubusercontent.com/u/346587?v=4","gravatar_id":"","url":"https://api.github.com/users/eteq","html_url":"https://github.com/eteq","followers_url":"https://api.github.com/users/eteq/followers","following_url":"https://api.github.com/users/eteq/following{/other_user}","gists_url":"https://api.github.com/users/eteq/gists{/gist_id}","starred_url":"https://api.github.com/users/eteq/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/eteq/subscriptions","organizations_url":"https://api.github.com/users/eteq/orgs","repos_url":"https://api.github.com/users/eteq/repos","events_url":"https://api.github.com/users/eteq/events{/privacy}","received_events_url":"https://api.github.com/users/eteq/received_events","type":"User","site_admin":false},"open_issues":79,"closed_issues":88,"state":"open","created_at":"2017-11-21T23:24:18Z","updated_at":"2018-07-23T17:17:59Z","due_on":null,"closed_at":null},"comments":50,"created_at":"2018-03-16T04:21:58Z","updated_at":"2018-07-23T18:40:40Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/astropy/astropy/pulls/7293","html_url":"https://github.com/astropy/astropy/pull/7293","diff_url":"https://github.com/astropy/astropy/pull/7293.diff","patch_url":"https://github.com/astropy/astropy/pull/7293.patch"},"body":"- [Convolution investigation and benchmark](https://github.com/jamienoss/astropy/issues/1)\r\n- [Performance Notes](https://github.com/jamienoss/astropy/issues/2)\r\n\r\n ## Perf. changes\r\n\r\n- Separate code for interpolating NaN values and when not. __Major perf. boost.__\r\n- If ``nan_treatment=‘interpolate’``, pre-check image for presence of NaN values,\r\n  if none exist, don’t call NaN interpolation code. __Major perf. boost.__\r\n- Hoist loop dependent variable computations to correct loop scope.\r\n- Untangle NaN interpolation and kernel normalization. Don’t normalize kernel until\r\n  after convolution instead of doing so before and again after if not requested.\r\n- Thread with OPENMP. Required replacing Cython code (.pyx) with raw C. __Major perf. boost.__\r\n- For ``boundary in [‘fill’, ‘extend’, ‘wrap’]`` use a padded array instead of specific index logic. __Major perf. boost.__\r\n  This has a significant performance boost, though it does increase the memory footprint.\r\n  It also signifcantly reduces the code base.\r\n- Improve DASK performance as previous Cython code used Python division and thus\r\n  meant reacquiring the GIL per loop iteration.\r\n  _Note:_ this could have been solved by adding the decorator: ``@cython.cdivision(True)`` to the Cython functions.\r\n  _Note:_ Using ``convolve`` threaded with DASK is still more performant than before however,\r\n  this will overcommit threads and could temporarily make the machine unresponsive. When using DASK\r\n  it is advised to make ``n_threads=1``, and can still improve runtime perf. by 2 compared to previous implementation.\r\n  [github issue](https://github.com/jamienoss/astropy/issues/5)\r\n- Hoist all exceptions to as early as possible, especially before memory allocation or heavy(ish) computations.\r\n\r\n ## Non Perf. changes\r\n\r\n- Correct doc string for default boundary case ``None`` -> ``’fill’``.\r\n\r\nSigned-off-by: James Noss <jnoss@stsci.edu>\r\n\r\n![pr-bench-figure](https://user-images.githubusercontent.com/5013975/37730059-0d5c4c8c-2d15-11e8-9ad1-f88a4e7fd897.png)\r\n\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/astropy/astropy/issues/comments/407159716","html_url":"https://github.com/astropy/astropy/pull/7293#issuecomment-407159716","issue_url":"https://api.github.com/repos/astropy/astropy/issues/7293","id":407159716,"node_id":"MDEyOklzc3VlQ29tbWVudDQwNzE1OTcxNg==","user":{"login":"jamienoss","id":5013975,"node_id":"MDQ6VXNlcjUwMTM5NzU=","avatar_url":"https://avatars2.githubusercontent.com/u/5013975?v=4","gravatar_id":"","url":"https://api.github.com/users/jamienoss","html_url":"https://github.com/jamienoss","followers_url":"https://api.github.com/users/jamienoss/followers","following_url":"https://api.github.com/users/jamienoss/following{/other_user}","gists_url":"https://api.github.com/users/jamienoss/gists{/gist_id}","starred_url":"https://api.github.com/users/jamienoss/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jamienoss/subscriptions","organizations_url":"https://api.github.com/users/jamienoss/orgs","repos_url":"https://api.github.com/users/jamienoss/repos","events_url":"https://api.github.com/users/jamienoss/events{/privacy}","received_events_url":"https://api.github.com/users/jamienoss/received_events","type":"User","site_admin":false},"created_at":"2018-07-23T18:40:39Z","updated_at":"2018-07-23T18:40:39Z","author_association":"CONTRIBUTOR","body":"@bsipocz Howdy. Should I have opened this against a release branch and not master? This PR is dependent on https://github.com/astropy/astropy-helpers/pull/382 which has been merged but the submodule in ``astropy`` is older. i.e. the required change is still not present to be used by this PR. Does this just need updating or...?"}}
{"actor":{"display_login":"jamienoss"}}