{"action":"created","comment":{"url":"https://api.github.com/repos/astropy/astropy/pulls/comments/154355119","pull_request_review_id":80508103,"id":154355119,"diff_hunk":"@@ -1283,19 +1284,50 @@ def _get_recarray_field(array, key):\n     return field\n \n \n-def _rstrip_inplace(array, chars=None):\n+def _rstrip_inplace(array):\n     \"\"\"\n-    Performs an in-place rstrip operation on string arrays.\n-    This is necessary since the built-in `np.char.rstrip` in Numpy does not\n-    perform an in-place calculation.  This can be removed if ever\n-    https://github.com/numpy/numpy/issues/6303 is implemented (however, for\n-    the purposes of this module the only in-place vectorized string functions\n-    we need are rstrip and encode).\n+    Performs an in-place rstrip operation on string arrays. This is necessary\n+    since the built-in `np.char.rstrip` in Numpy does not perform an in-place\n+    calculation.\n     \"\"\"\n \n-    for item in np.nditer(array, flags=['zerosize_ok'],\n-                                 op_flags=['readwrite']):\n-        item[...] = item.item().rstrip(chars)\n+    # The following implementation convert the string to unsigned integers of\n+    # the right length. Trailing spaces (which are represented as 32) are then\n+    # converted to null characters (represented as zeros). To avoid creating\n+    # large temporary mask arrays, we loop over chunks (attempting to do that\n+    # on a 1-D version of the array; large memomry may still be needed in the\n+    # unlikely case that a string array has small first dimension and cannot\n+    # be represented as a contiguous 1-D array in memory).\n+\n+    dt = array.dtype\n+\n+    if dt.kind not in 'SU':\n+        raise TypeError(\"This function can only be used on string arrays\")\n+    bpc = 1 if dt.kind == 'S' else 4\n+    dt_int = \"{0}{1}u{2}\".format(dt.itemsize // bpc, dt.byteorder, dt.bpc)","path":"astropy/io/fits/fitsrec.py","position":51,"original_position":51,"commit_id":"56d8cf53d134d78ca133efa4a058c38afea9400e","original_commit_id":"56d8cf53d134d78ca133efa4a058c38afea9400e","user":{"login":"saimn","id":311639,"avatar_url":"https://avatars0.githubusercontent.com/u/311639?v=4","gravatar_id":"","url":"https://api.github.com/users/saimn","html_url":"https://github.com/saimn","followers_url":"https://api.github.com/users/saimn/followers","following_url":"https://api.github.com/users/saimn/following{/other_user}","gists_url":"https://api.github.com/users/saimn/gists{/gist_id}","starred_url":"https://api.github.com/users/saimn/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/saimn/subscriptions","organizations_url":"https://api.github.com/users/saimn/orgs","repos_url":"https://api.github.com/users/saimn/repos","events_url":"https://api.github.com/users/saimn/events{/privacy}","received_events_url":"https://api.github.com/users/saimn/received_events","type":"User","site_admin":false},"body":"Should use `bpc` instead of `dt.bpc`.","created_at":"2017-12-01T14:18:20Z","updated_at":"2017-12-01T14:27:27Z","html_url":"https://github.com/astropy/astropy/pull/6920#discussion_r154355119","pull_request_url":"https://api.github.com/repos/astropy/astropy/pulls/6920","author_association":"CONTRIBUTOR","_links":{"self":{"href":"https://api.github.com/repos/astropy/astropy/pulls/comments/154355119"},"html":{"href":"https://github.com/astropy/astropy/pull/6920#discussion_r154355119"},"pull_request":{"href":"https://api.github.com/repos/astropy/astropy/pulls/6920"}}},"pull_request":{"url":"https://api.github.com/repos/astropy/astropy/pulls/6920","id":155845967,"html_url":"https://github.com/astropy/astropy/pull/6920","diff_url":"https://github.com/astropy/astropy/pull/6920.diff","patch_url":"https://github.com/astropy/astropy/pull/6920.patch","issue_url":"https://api.github.com/repos/astropy/astropy/issues/6920","number":6920,"state":"open","locked":false,"title":"WIP: Speed up writing of FITS tables with string columns","user":{"login":"astrofrog","id":314716,"avatar_url":"https://avatars2.githubusercontent.com/u/314716?v=4","gravatar_id":"","url":"https://api.github.com/users/astrofrog","html_url":"https://github.com/astrofrog","followers_url":"https://api.github.com/users/astrofrog/followers","following_url":"https://api.github.com/users/astrofrog/following{/other_user}","gists_url":"https://api.github.com/users/astrofrog/gists{/gist_id}","starred_url":"https://api.github.com/users/astrofrog/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/astrofrog/subscriptions","organizations_url":"https://api.github.com/users/astrofrog/orgs","repos_url":"https://api.github.com/users/astrofrog/repos","events_url":"https://api.github.com/users/astrofrog/events{/privacy}","received_events_url":"https://api.github.com/users/astrofrog/received_events","type":"User","site_admin":false},"body":"*Still a work in progress but initial comments/testing welcome*\r\n\r\nThis is a follow-up to #6821 and speeds up specifically writing tables with string columns.\r\n\r\nAt the moment, if a table contains a string column, that string column is first decoded by io.fits to a unicode array (if not already the case) before getting re-encoded to a byte array. This means that if the original column was a byte string column (which one could get after reading in a FITS file with Table.read), there is an unnecessary conversion to unicode and back. Furthermore, there was previously a Python loop over all string elements to remove trailing whitespace. This PR uses the ``character_as_bytes`` functionality developed in #6821 to avoid the conversion to unicode in the case a byte string array is passed, and also includes a significantly sped up ``_rstrip_replace`` provided by @mhvk in https://github.com/astropy/astropy/issues/6906.\r\n\r\n@mhvk - I feel bad including your code without assigning the commit to you, so if you want the credit, feel free to edit this PR and change the existing commit to be under your user!\r\n\r\nTest code:\r\n\r\n```python\r\nimport numpy as np\r\nfrom astropy.table import Table\r\n\r\nN = 10_000_000\r\n\r\nt = Table()\r\nt['floats1'] = np.random.random(N)\r\nt['ints1'] = np.random.randint(0, 100, N)\r\nt['strings'] = b'some strings'\r\n\r\nt.write('test_write.fits', overwrite=True)\r\n```\r\n\r\nThe table initialization takes 1.5s, and the writing of the table itself takes 22.1s before this PR and 2.1s after, giving a speedup of a factor of 10x, and the peak memory usage goes down by a factor of ~2.5x:\r\n\r\n![perf](https://user-images.githubusercontent.com/314716/33481186-53b06654-d68b-11e7-98e9-2189c376cb05.png)\r\n\r\n(the 'speed up ``_rstrip_replace``' curve also includes ``character_as_bytes``)\r\n\r\nIf I set ``t['strings']`` to a unicode column instead of bytes column:\r\n\r\n```\r\nt['strings'] = 'some strings'\r\n```\r\n\r\nThings are also faster with this PR:\r\n\r\n![perf_u](https://user-images.githubusercontent.com/314716/33481456-778a4ad0-d68c-11e7-8c63-c1d811747864.png)\r\n\r\n(the 'speed up ``_rstrip_replace``' curve also includes ``character_as_bytes``)\r\n\r\nIn both cases writing a table still seems to double the memory used by the table, but I'm not sure if we can avoid that. One use case I'd like to test is whether if I have a larger-than-memory table I can read it in with memory mapping, modify a cell, and write it out again without using much memory.\r\n\r\nNow we might want to consider whether to put some of the optimizations lower down in io.fits - for instance if doing ``BinTableHDU.from_columns`` with a byte array, it will be converted to unicode internally then back to bytes as mentioned above. Check this out:\r\n\r\n```python\r\nimport numpy as np\r\nfrom astropy.io.fits import BinTableHDU\r\nx = np.repeat(b'a', 100_000_000)\r\narray = np.array(x, dtype=[('col', 'S1')])\r\nhdu = BinTableHDU.from_columns(array)\r\nprint(hdu.dtype)\r\n````\r\n\r\nThis takes almost a minute and 2.2Gb of memory:\r\n\r\n![hdu_false](https://user-images.githubusercontent.com/314716/33481918-8a96fb1c-d68e-11e7-95d9-b97731d4a288.png)\r\n\r\nThe output for the dtype is ``(numpy.record, [('col', 'S1')])``. If I call it with:\r\n\r\n```python\r\nhdu = BinTableHDU.from_columns(array, character_as_bytes=True)\r\n```\r\n\r\nit takes 2 seconds and 350Mb of memory:\r\n\r\n![hdu_true](https://user-images.githubusercontent.com/314716/33481916-87f89a50-d68e-11e7-8a3a-b9d7b55f749b.png)\r\n\r\nAnd produces the exact same output. So maybe ``BinTableHDU.from_columns`` shouldn't even have a ``character_as_bytes`` option and it should always default to ``True``. But I need to think about the implications about this some more and would value input from FITS experts (e.g. @saimn @MSeifert04).","created_at":"2017-12-01T11:59:36Z","updated_at":"2017-12-01T14:27:27Z","closed_at":null,"merged_at":null,"merge_commit_sha":"e19e80b8df9c5c7773a3e328c2358eaa0af96bbe","assignee":null,"assignees":[],"requested_reviewers":[],"milestone":null,"commits_url":"https://api.github.com/repos/astropy/astropy/pulls/6920/commits","review_comments_url":"https://api.github.com/repos/astropy/astropy/pulls/6920/comments","review_comment_url":"https://api.github.com/repos/astropy/astropy/pulls/comments{/number}","comments_url":"https://api.github.com/repos/astropy/astropy/issues/6920/comments","statuses_url":"https://api.github.com/repos/astropy/astropy/statuses/56d8cf53d134d78ca133efa4a058c38afea9400e","head":{"label":"astrofrog:table-fits-writing-speedup","ref":"table-fits-writing-speedup","sha":"56d8cf53d134d78ca133efa4a058c38afea9400e","user":{"login":"astrofrog","id":314716,"avatar_url":"https://avatars2.githubusercontent.com/u/314716?v=4","gravatar_id":"","url":"https://api.github.com/users/astrofrog","html_url":"https://github.com/astrofrog","followers_url":"https://api.github.com/users/astrofrog/followers","following_url":"https://api.github.com/users/astrofrog/following{/other_user}","gists_url":"https://api.github.com/users/astrofrog/gists{/gist_id}","starred_url":"https://api.github.com/users/astrofrog/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/astrofrog/subscriptions","organizations_url":"https://api.github.com/users/astrofrog/orgs","repos_url":"https://api.github.com/users/astrofrog/repos","events_url":"https://api.github.com/users/astrofrog/events{/privacy}","received_events_url":"https://api.github.com/users/astrofrog/received_events","type":"User","site_admin":false},"repo":{"id":2576841,"name":"astropy","full_name":"astrofrog/astropy","owner":{"login":"astrofrog","id":314716,"avatar_url":"https://avatars2.githubusercontent.com/u/314716?v=4","gravatar_id":"","url":"https://api.github.com/users/astrofrog","html_url":"https://github.com/astrofrog","followers_url":"https://api.github.com/users/astrofrog/followers","following_url":"https://api.github.com/users/astrofrog/following{/other_user}","gists_url":"https://api.github.com/users/astrofrog/gists{/gist_id}","starred_url":"https://api.github.com/users/astrofrog/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/astrofrog/subscriptions","organizations_url":"https://api.github.com/users/astrofrog/orgs","repos_url":"https://api.github.com/users/astrofrog/repos","events_url":"https://api.github.com/users/astrofrog/events{/privacy}","received_events_url":"https://api.github.com/users/astrofrog/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/astrofrog/astropy","description":"Main AstroPy repository","fork":true,"url":"https://api.github.com/repos/astrofrog/astropy","forks_url":"https://api.github.com/repos/astrofrog/astropy/forks","keys_url":"https://api.github.com/repos/astrofrog/astropy/keys{/key_id}","collaborators_url":"https://api.github.com/repos/astrofrog/astropy/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/astrofrog/astropy/teams","hooks_url":"https://api.github.com/repos/astrofrog/astropy/hooks","issue_events_url":"https://api.github.com/repos/astrofrog/astropy/issues/events{/number}","events_url":"https://api.github.com/repos/astrofrog/astropy/events","assignees_url":"https://api.github.com/repos/astrofrog/astropy/assignees{/user}","branches_url":"https://api.github.com/repos/astrofrog/astropy/branches{/branch}","tags_url":"https://api.github.com/repos/astrofrog/astropy/tags","blobs_url":"https://api.github.com/repos/astrofrog/astropy/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/astrofrog/astropy/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/astrofrog/astropy/git/refs{/sha}","trees_url":"https://api.github.com/repos/astrofrog/astropy/git/trees{/sha}","statuses_url":"https://api.github.com/repos/astrofrog/astropy/statuses/{sha}","languages_url":"https://api.github.com/repos/astrofrog/astropy/languages","stargazers_url":"https://api.github.com/repos/astrofrog/astropy/stargazers","contributors_url":"https://api.github.com/repos/astrofrog/astropy/contributors","subscribers_url":"https://api.github.com/repos/astrofrog/astropy/subscribers","subscription_url":"https://api.github.com/repos/astrofrog/astropy/subscription","commits_url":"https://api.github.com/repos/astrofrog/astropy/commits{/sha}","git_commits_url":"https://api.github.com/repos/astrofrog/astropy/git/commits{/sha}","comments_url":"https://api.github.com/repos/astrofrog/astropy/comments{/number}","issue_comment_url":"https://api.github.com/repos/astrofrog/astropy/issues/comments{/number}","contents_url":"https://api.github.com/repos/astrofrog/astropy/contents/{+path}","compare_url":"https://api.github.com/repos/astrofrog/astropy/compare/{base}...{head}","merges_url":"https://api.github.com/repos/astrofrog/astropy/merges","archive_url":"https://api.github.com/repos/astrofrog/astropy/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/astrofrog/astropy/downloads","issues_url":"https://api.github.com/repos/astrofrog/astropy/issues{/number}","pulls_url":"https://api.github.com/repos/astrofrog/astropy/pulls{/number}","milestones_url":"https://api.github.com/repos/astrofrog/astropy/milestones{/number}","notifications_url":"https://api.github.com/repos/astrofrog/astropy/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/astrofrog/astropy/labels{/name}","releases_url":"https://api.github.com/repos/astrofrog/astropy/releases{/id}","deployments_url":"https://api.github.com/repos/astrofrog/astropy/deployments","created_at":"2011-10-14T14:58:26Z","updated_at":"2016-12-20T14:00:17Z","pushed_at":"2017-12-01T13:46:52Z","git_url":"git://github.com/astrofrog/astropy.git","ssh_url":"git@github.com:astrofrog/astropy.git","clone_url":"https://github.com/astrofrog/astropy.git","svn_url":"https://github.com/astrofrog/astropy","homepage":"astropy.org","size":57271,"stargazers_count":4,"watchers_count":4,"language":"Python","has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"archived":false,"open_issues_count":0,"license":null,"forks":0,"open_issues":0,"watchers":4,"default_branch":"master"}},"base":{"label":"astropy:master","ref":"master","sha":"8ad19c68ddbc2ac33eaf90c1ff988991bbdcc745","user":{"login":"astropy","id":847984,"avatar_url":"https://avatars0.githubusercontent.com/u/847984?v=4","gravatar_id":"","url":"https://api.github.com/users/astropy","html_url":"https://github.com/astropy","followers_url":"https://api.github.com/users/astropy/followers","following_url":"https://api.github.com/users/astropy/following{/other_user}","gists_url":"https://api.github.com/users/astropy/gists{/gist_id}","starred_url":"https://api.github.com/users/astropy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/astropy/subscriptions","organizations_url":"https://api.github.com/users/astropy/orgs","repos_url":"https://api.github.com/users/astropy/repos","events_url":"https://api.github.com/users/astropy/events{/privacy}","received_events_url":"https://api.github.com/users/astropy/received_events","type":"Organization","site_admin":false},"repo":{"id":2081289,"name":"astropy","full_name":"astropy/astropy","owner":{"login":"astropy","id":847984,"avatar_url":"https://avatars0.githubusercontent.com/u/847984?v=4","gravatar_id":"","url":"https://api.github.com/users/astropy","html_url":"https://github.com/astropy","followers_url":"https://api.github.com/users/astropy/followers","following_url":"https://api.github.com/users/astropy/following{/other_user}","gists_url":"https://api.github.com/users/astropy/gists{/gist_id}","starred_url":"https://api.github.com/users/astropy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/astropy/subscriptions","organizations_url":"https://api.github.com/users/astropy/orgs","repos_url":"https://api.github.com/users/astropy/repos","events_url":"https://api.github.com/users/astropy/events{/privacy}","received_events_url":"https://api.github.com/users/astropy/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/astropy/astropy","description":"Repository for the Astropy core package","fork":false,"url":"https://api.github.com/repos/astropy/astropy","forks_url":"https://api.github.com/repos/astropy/astropy/forks","keys_url":"https://api.github.com/repos/astropy/astropy/keys{/key_id}","collaborators_url":"https://api.github.com/repos/astropy/astropy/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/astropy/astropy/teams","hooks_url":"https://api.github.com/repos/astropy/astropy/hooks","issue_events_url":"https://api.github.com/repos/astropy/astropy/issues/events{/number}","events_url":"https://api.github.com/repos/astropy/astropy/events","assignees_url":"https://api.github.com/repos/astropy/astropy/assignees{/user}","branches_url":"https://api.github.com/repos/astropy/astropy/branches{/branch}","tags_url":"https://api.github.com/repos/astropy/astropy/tags","blobs_url":"https://api.github.com/repos/astropy/astropy/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/astropy/astropy/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/astropy/astropy/git/refs{/sha}","trees_url":"https://api.github.com/repos/astropy/astropy/git/trees{/sha}","statuses_url":"https://api.github.com/repos/astropy/astropy/statuses/{sha}","languages_url":"https://api.github.com/repos/astropy/astropy/languages","stargazers_url":"https://api.github.com/repos/astropy/astropy/stargazers","contributors_url":"https://api.github.com/repos/astropy/astropy/contributors","subscribers_url":"https://api.github.com/repos/astropy/astropy/subscribers","subscription_url":"https://api.github.com/repos/astropy/astropy/subscription","commits_url":"https://api.github.com/repos/astropy/astropy/commits{/sha}","git_commits_url":"https://api.github.com/repos/astropy/astropy/git/commits{/sha}","comments_url":"https://api.github.com/repos/astropy/astropy/comments{/number}","issue_comment_url":"https://api.github.com/repos/astropy/astropy/issues/comments{/number}","contents_url":"https://api.github.com/repos/astropy/astropy/contents/{+path}","compare_url":"https://api.github.com/repos/astropy/astropy/compare/{base}...{head}","merges_url":"https://api.github.com/repos/astropy/astropy/merges","archive_url":"https://api.github.com/repos/astropy/astropy/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/astropy/astropy/downloads","issues_url":"https://api.github.com/repos/astropy/astropy/issues{/number}","pulls_url":"https://api.github.com/repos/astropy/astropy/pulls{/number}","milestones_url":"https://api.github.com/repos/astropy/astropy/milestones{/number}","notifications_url":"https://api.github.com/repos/astropy/astropy/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/astropy/astropy/labels{/name}","releases_url":"https://api.github.com/repos/astropy/astropy/releases{/id}","deployments_url":"https://api.github.com/repos/astropy/astropy/deployments","created_at":"2011-07-21T01:33:49Z","updated_at":"2017-11-30T13:27:12Z","pushed_at":"2017-12-01T14:12:38Z","git_url":"git://github.com/astropy/astropy.git","ssh_url":"git@github.com:astropy/astropy.git","clone_url":"https://github.com/astropy/astropy.git","svn_url":"https://github.com/astropy/astropy","homepage":"www.astropy.org","size":62875,"stargazers_count":1476,"watchers_count":1476,"language":"Python","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":783,"mirror_url":null,"archived":false,"open_issues_count":812,"license":{"key":"bsd-3-clause","name":"BSD 3-clause \"New\" or \"Revised\" License","spdx_id":"BSD-3-Clause","url":"https://api.github.com/licenses/bsd-3-clause"},"forks":783,"open_issues":812,"watchers":1476,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/astropy/astropy/pulls/6920"},"html":{"href":"https://github.com/astropy/astropy/pull/6920"},"issue":{"href":"https://api.github.com/repos/astropy/astropy/issues/6920"},"comments":{"href":"https://api.github.com/repos/astropy/astropy/issues/6920/comments"},"review_comments":{"href":"https://api.github.com/repos/astropy/astropy/pulls/6920/comments"},"review_comment":{"href":"https://api.github.com/repos/astropy/astropy/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/astropy/astropy/pulls/6920/commits"},"statuses":{"href":"https://api.github.com/repos/astropy/astropy/statuses/56d8cf53d134d78ca133efa4a058c38afea9400e"}},"author_association":"OWNER"}}
{"actor":{"display_login":"saimn"}}