using frombuffer instead of fromstring by bsipocz · Pull Request #6785 · astropy/astropy

bsipocz · 2017-10-24T14:05:38Z

This is to fix #6784 as a follow-up for recent numpy deprecations (numpy/numpy#9487).

I'm not sure whether I choose the right encoding for the fits modifications, but requesting a review from the maintainers to advise otherwise.

If possible, this also needs to be backported.

astropy-bot · 2017-10-24T14:05:40Z

Hi there @bsipocz 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here

pllim

Seems alright for io.votable but let me take it for a spin with my cron job and I'll approve if nothing crashes tomorrow.

mhvk · 2017-10-24T18:31:59Z

        # their underlying file object, instead of the decompressed bytes
        read_size = np.dtype(dtype).itemsize * count
        s = infile.read(read_size)
-        return np.fromstring(s, dtype=dtype, count=count, sep=sep)


This may be wrong: is sep ever given as a real string? Certainly, fromfile can handle both an empty string (binary) and a given separation string (text). Unless we are sure sep is always '', we probably are stuck with

if sep == '': return np.frombuffer(...) else: return np.fromstring(..., sep=sep)

OK, I've added this back (GH just doesn't recognize it).

Should be OK now, thanks!

sep is always used with an empty string so I think it's safe to simplify this, see #6813

mhvk · 2017-10-24T18:35:58Z

    nblanks = input.count(' ')
    nmax = max(nblanks, len(input) // strlen + 1)
-    arr = np.fromstring((input + ' '), dtype=(bytes, 1))
+    arr = np.frombuffer((input + ' ').encode('utf8'), dtype=(bytes, 1))


This is quite ugly; really should rewrite the below without using an array. But that's beyond the scope of this PR...

I was reading this while looking at other uses of np.frombuffer, and actually I wonder why this function exists instead of using textwrap.wrap... maybe some weird edge case that I don't see, but on a simple string (as tested with test_long_string_value it gives the same result).

pllim

Cron job didn't crash, so 👍 for io.votable changes.

MSeifert04 · 2017-10-25T16:18:28Z

I'll have a look at the PR wrt to io.fits this evening :)

MSeifert04 · 2017-10-25T23:51:16Z

-        # Convert the header to a string.
-        s = str(self._header)
+        # Convert the header to bytes.
+        s = self._header.tostring().encode('utf8')


I thinks it's impossible to pass in non-ascii (or non-latin-1) characters in a Header. However the binary representation of these should be the same as for utf-8 so there's (probably) no problem.

MSeifert04 · 2017-10-25T23:51:56Z

    Split a long string into parts where each part is no longer
-    than `strlen` and no word is cut into two pieces.  But if
-    there is one single word which is longer than `strlen`, then
+    than ``strlen`` and no word is cut into two pieces.  But if


Good catch!

pllim · 2017-10-26T00:55:12Z

We have 3 favorable reviews, so merging. Thanks!

Following astropy#6785, `sep` is always an empty string so no need to keep the `np.fromstring` case.

using frombuffer instead of fromstring

Fix astropy#6862. astropy#6785 replaced uses of np.fromstring with np.frombuffer but the latter returns a read-only view of the binary buffer. So copying the array is needed to get a writeable array (and np.fromstring was doing the copy internally).

saimn · 2017-12-04T09:40:37Z

    def binparse(self, read):
-        result = np.fromstring(read(self._memsize),
+        result = np.frombuffer(read(self._memsize),
                               dtype=self._bigendian_format)[0]


The two uses of np.frombuffer here should be checked to see if it's safe if result a read-only array, (or if something is untested). poke @pllim 😉

Consider me poked. I'll investigate and report back. (Also, whoever wants to be a maintainer for votable, feel free to volunteer...) 😉

pllim · 2017-12-06T21:31:26Z

@saimn , from what I can understand, it does not seem to be a problem for votable. More details below:

https://coveralls.io/builds/14536008/source?filename=astropy%2Fio%2Fvotable%2Fconverters.py indicates np.frombuffer is covered in testing.
np.frombuffer is used in binparse() methods for various classes in converters.py, which in turn is used in _parse_tabledata() and _parse_binary() methods in tree.py. In both _parse_* methods, there is a call to array = _resize(array, ...) at the end (though it does not look like it is making a copy?).
Just to be sure, I manually checked if the resultant array is read-only but does not look like it. See examples below using astropy/io/votable/tests/data/regression.xml.

Example 1 -- Using astropy.table interface:

>>> from astropy.table import Table
>>> tab = Table.read('regression.xml', table_id=0)
>>> tab
<Table masked=True length=5>
   string_test    string_test_2 ... bitarray2 [16]
      object         bytes10    ...      bool     
----------------- ------------- ... --------------
    String & test    Fixed stri ...  True .. False
String &amp; test    0123456789 ...       -- .. --
             XXXX          XXXX ...       -- .. --
                                ...       -- .. --
                                ...       -- .. --
>>> tab['string_test'][0] = 'stringcheese'
>>> tab
<Table masked=True length=5>
   string_test    string_test_2 ... bitarray2 [16]
      object         bytes10    ...      bool     
----------------- ------------- ... --------------
     stringcheese    Fixed stri ...  True .. False
String &amp; test    0123456789 ...       -- .. --
             XXXX          XXXX ...       -- .. --
                                ...       -- .. --
                                ...       -- .. --
>>> tab.write('mystringcheese.xml', format='votable')  # Gives me the modified table

Example 2 -- Using votable interface:

>>> from astropy.io.votable.table import parse
>>> tt = parse('regression.xml')
>>> tab = tt.get_first_table()
>>> tab
<Table masked=True length=5>
   string_test    string_test_2 ... bitarray2 [16]
      object         bytes10    ...      bool     
----------------- ------------- ... --------------
    String & test    Fixed stri ...  True .. False
String &amp; test    0123456789 ...       -- .. --
             XXXX          XXXX ...       -- .. --
                                ...       -- .. --
                                ...       -- .. --
>>> tab.array[0][0] = b'stringcheese'
>>> tab
<Table masked=True length=5>
   string_test    string_test_2 ... bitarray2 [16]
      object         bytes10    ...      bool     
----------------- ------------- ... --------------
     stringcheese    Fixed stri ...  True .. False
String &amp; test    0123456789 ...       -- .. --
             XXXX          XXXX ...       -- .. --
                                ...       -- .. --
                                ...       -- .. --

Is this convincing enough? If not, please suggest other ways I could test this. Thanks!

saimn · 2017-12-06T21:39:44Z

_resize allocates a new array so it looks good indeed. Thanks for having verified 😉

bsipocz added 3 commits October 24, 2017 11:55

Using frombuffer rather than fromstring in votable

e430af6

using frombuffer instead of fromstring in io.fits

1a8458b

Using bytes instead of string

acabaa7

bsipocz added io.fits io.votable no-changelog-entry-needed numpy-dev labels Oct 24, 2017

bsipocz added this to the v2.0.3 milestone Oct 24, 2017

bsipocz requested review from MSeifert04, pllim and saimn October 24, 2017 14:05

pllim reviewed Oct 24, 2017

View reviewed changes

mhvk reviewed Oct 24, 2017

View reviewed changes

drdavella mentioned this pull request Oct 24, 2017

Relax requirement on semantic_version version to 2.3.1 asdf-format/asdf#361

Merged

pllim approved these changes Oct 25, 2017

View reviewed changes

Adding back handling of case with a separator string

1cbca23

MSeifert04 reviewed Oct 25, 2017

View reviewed changes

MSeifert04 approved these changes Oct 25, 2017

View reviewed changes

pllim merged commit 7334480 into astropy:master Oct 26, 2017

saimn added a commit to saimn/astropy that referenced this pull request Nov 6, 2017

Remove the sep argument from _array_from_file

79986cd

Following astropy#6785, `sep` is always an empty string so no need to keep the `np.fromstring` case.

saimn mentioned this pull request Nov 6, 2017

Remove the sep argument from _array_from_file #6813

Merged

bsipocz pushed a commit that referenced this pull request Nov 7, 2017

Merge pull request #6785 from bsipocz/frombuffer_instead_of_fromstring

04a2004

using frombuffer instead of fromstring

saimn mentioned this pull request Nov 16, 2017

Gzipped FITS BinTableHDU readonly now #6862

Closed

saimn mentioned this pull request Dec 3, 2017

Fix use of frombuffer which made array read-only #6923

Merged

saimn reviewed Dec 4, 2017

View reviewed changes

pllim mentioned this pull request Jan 10, 2018

IRSA has changed URLs from http to https astropy/astroquery#1010

Merged

bsipocz deleted the frombuffer_instead_of_fromstring branch September 16, 2024 21:16

Uh oh!

Uh oh!

Conversation

bsipocz commented Oct 24, 2017

Uh oh!

astropy-bot Bot commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Oct 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saimn commented Dec 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

astropy-bot Bot commented Oct 24, 2017 •

edited

Loading

pllim commented Dec 6, 2017 •

edited

Loading