RF: Circumvents a deprecation warning from `np.fromstring` #702

arokem · 2018-12-17T21:21:59Z

I am seeing this warning:

/Users/arokem/.virtualenvs/afq/lib/python3.7/site-packages/nibabel/streamlines/trk.py:562: DeprecationWarning: The binary mode of fromstring is deprecated, as it behaves surprisingly on unicode inputs. Use frombuffer instead
  header_rec = np.fromstring(string=header_str, dtype=header_2_dtype)

I believe this PR fixes that.

effigies · 2018-12-17T21:31:07Z

Thanks. Looks like frombuffer() has the same API back at least to our minimum numpy, so this seems reasonable. I think there's a chance you'll have trouble setting the write flag on the pre-release numpy, so we might not be done yet.

Any chance you could swap out numpy.fromstring() in the rest of the codebase, while you're at it?

arokem · 2018-12-17T21:39:09Z

Any chance you could swap out numpy.fromstring() in the rest of the codebase <https://github.com/nipy/nibabel/search?q=fromstring&unscoped_q=fromstring>, while you're at it?

Yep. Will do.

coveralls · 2018-12-17T21:43:43Z

Coverage increased (+0.002%) to 91.83% when pulling 6924a56 on arokem:fromstring_deprecation into 3652f16 on nipy:master.

arokem · 2018-12-17T21:46:26Z

As anticipated, pre-release numpy doesn't allow setting this array to writeable. Do you know if there is any fix to this?

effigies · 2018-12-17T21:49:56Z

You'll need to readinto some read-write object, such as a bytearray. See discussions in #697, #700.

codecov-io · 2018-12-17T21:59:00Z

Codecov Report

Merging #702 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #702      +/-   ##
==========================================
+ Coverage   88.85%   88.88%   +0.03%     
==========================================
  Files          93       93              
  Lines       11454    11452       -2     
  Branches     1894     1892       -2     
==========================================
+ Hits        10177    10179       +2     
+ Misses        936      933       -3     
+ Partials      341      340       -1

Impacted Files	Coverage Δ
nibabel/openers.py	`76.85% <100%> (+0.38%)`	⬆️
nibabel/gifti/parse_gifti_fast.py	`84.51% <100%> (ø)`	⬆️
nibabel/streamlines/trk.py	`94.31% <100%> (+0.01%)`	⬆️
nibabel/nifti1.py	`91.22% <100%> (ø)`	⬆️
nibabel/info.py	`100% <0%> (ø)`	⬆️
nibabel/viewers.py	`96.66% <0%> (+1.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3652f16...6924a56. Read the comment docs.

arokem · 2018-12-17T22:12:59Z

OK. I believe that remaining appearances of fromstring are not of the deprecated kind.

effigies

Great! Thanks. We can be a little more efficient with the TrkFile header.

As for the other calls, as long as tests pass and coverage hits them, I think we should be safe.

effigies · 2018-12-17T23:02:00Z

nibabel/streamlines/trk.py

@@ -559,8 +559,8 @@ def _read_header(fileobj):

            # Read the header in one block.
            header_str = f.read(header_2_dtype.itemsize)
-            header_rec = np.fromstring(string=header_str, dtype=header_2_dtype)
-
+            header_rec = np.frombuffer(buffer=bytearray(header_str),


This still makes a copy from header_str to bytearray(header_str). Instead, we can create a bytearray and read directly into it:

header_buf = bytearray(header_2_dtype.itemsize) n_read = f.readinto(header_buf) # Maybe if n_read != header_2_dtype.itemsize: raise HeaderError(...) header_rec = np.frombuffer(buffer=header_buf, dtype=header_2_dtype)

arokem · 2018-12-17T23:29:45Z

But the `Opener` object used here doesn't have a `readinto` buffered read method implemented, as far as I can understand.

…

On Mon, Dec 17, 2018 at 3:04 PM Chris Markiewicz ***@***.***> wrote: ***@***.**** commented on this pull request. Great! Thanks. We can be a little more efficient with the TrkFile header. As for the other calls, as long as tests pass and coverage hits them, I think we should be safe. ------------------------------ In nibabel/streamlines/trk.py <#702 (comment)>: > @@ -559,8 +559,8 @@ def _read_header(fileobj): # Read the header in one block. header_str = f.read(header_2_dtype.itemsize) - header_rec = np.fromstring(string=header_str, dtype=header_2_dtype) - + header_rec = np.frombuffer(buffer=bytearray(header_str), This still makes a copy from header_str to bytearray(header_str). Instead, we can create a bytearray and read directly into it: header_buf = bytearray(header_2_dtype.itemsize) n_read = f.readinto(header_buf) # Maybe if n_read != header_2_dtype.itemsize: raise HeaderError(...) header_rec = np.frombuffer(buffer=header_buf, dtype=header_2_dtype) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#702 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNsDQ1FR_g7ByQLFhjOP0Rst0FtEsks5u6CMFgaJpZM4ZXJsu> .

arokem · 2018-12-17T23:38:33Z

Oh, scratch that. Now it does :-)

…

On Mon, Dec 17, 2018 at 3:29 PM Ariel Rokem ***@***.***> wrote: But the `Opener` object used here doesn't have a `readinto` buffered read method implemented, as far as I can understand. On Mon, Dec 17, 2018 at 3:04 PM Chris Markiewicz ***@***.***> wrote: > ***@***.**** commented on this pull request. > > Great! Thanks. We can be a little more efficient with the TrkFile header. > > As for the other calls, as long as tests pass and coverage hits them, I > think we should be safe. > ------------------------------ > > In nibabel/streamlines/trk.py > <#702 (comment)>: > > > @@ -559,8 +559,8 @@ def _read_header(fileobj): > > # Read the header in one block. > header_str = f.read(header_2_dtype.itemsize) > - header_rec = np.fromstring(string=header_str, dtype=header_2_dtype) > - > + header_rec = np.frombuffer(buffer=bytearray(header_str), > > This still makes a copy from header_str to bytearray(header_str). > Instead, we can create a bytearray and read directly into it: > > header_buf = bytearray(header_2_dtype.itemsize) > n_read = f.readinto(header_buf) > # Maybe if n_read != header_2_dtype.itemsize: raise HeaderError(...) > header_rec = np.frombuffer(buffer=header_buf, dtype=header_2_dtype) > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#702 (review)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAHPNsDQ1FR_g7ByQLFhjOP0Rst0FtEsks5u6CMFgaJpZM4ZXJsu> > . >

effigies · 2018-12-18T01:05:51Z

nibabel/openers.py

@@ -209,6 +209,9 @@ def fileno(self):
    def read(self, *args, **kwargs):
        return self.fobj.read(*args, **kwargs)

+    def readinto(self, *args, **kwargs):
+        return self.fobj.readinto(*args, **kwargs)
+


Oh, nice. I hadn't realized we didn't already have this...

arokem · 2018-12-18T03:16:40Z

Any suggestions on how to test that? I got a bit lost in test_openers

…

On Mon, Dec 17, 2018 at 5:05 PM Chris Markiewicz ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In nibabel/openers.py <#702 (comment)>: > @@ -209,6 +209,9 @@ def fileno(self): def read(self, *args, **kwargs): return self.fobj.read(*args, **kwargs) + def readinto(self, *args, **kwargs): + return self.fobj.readinto(*args, **kwargs) + Oh, nice. I hadn't realized we didn't already have this... — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#702 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHPNgB6TGERGdJtaTQja80Ho4NDwqFtks5u6D9wgaJpZM4ZXJsu> .

effigies · 2018-12-18T14:03:09Z

I think this function is as close as we come to fully testing the API for equivalence:

nibabel/nibabel/tests/test_openers.py

Lines 198 to 213 in a960b92

    
           def test_file_like_wrapper(): 
        
               # Test wrapper using BytesIO (full API) 
        
               message = b"History of the nude in" 
        
               sobj = BytesIO() 
        
               fobj = Opener(sobj) 
        
               assert_equal(fobj.tell(), 0) 
        
               fobj.write(message) 
        
               assert_equal(fobj.tell(), len(message)) 
        
               fobj.seek(0) 
        
               assert_equal(fobj.tell(), 0) 
        
               assert_equal(fobj.read(6), message[:6]) 
        
               assert_false(fobj.closed) 
        
               fobj.close() 
        
               assert_true(fobj.closed) 
        
               # Added the fileobj name 
        
               assert_equal(fobj.name, None)

arokem · 2018-12-18T18:46:36Z

Looks like we have similar issues in other places as well. Let me see if I can use similar approaches in these other places as well...

effigies · 2018-12-18T19:45:29Z

Awesome, thanks. In #700, I'll be fixing the volumeutils piece to use bz2file, which implements readinto and should take care of that one, but the TCK and TRK ones could use your attention.

arokem · 2018-12-18T19:52:23Z

OK. I'll look into those.

arokem · 2018-12-26T21:17:46Z

Sorry: I have not been able to resolve these other spots and might not have time to work on this any time soon. If it's OK, I would suggest merging this one, and then keep working on the other ones on another PR.

``` /Users/arokem/.virtualenvs/afq/lib/python3.7/site-packages/nibabel/streamlines/trk.py:562: DeprecationWarning: The binary mode of fromstring is deprecated, as it behaves surprisingly on unicode inputs. Use frombuffer instead header_rec = np.fromstring(string=header_str, dtype=header_2_dtype) ```

This is because newer numpy doesn't allow to change the writeable flag.

effigies · 2018-12-27T00:23:43Z

@arokem I've merged your branch into #700, as I needed some of your improvements to finish the job. Should be just about ready to merge over there. This PR will automatically close when that one is merged.

arokem · 2018-12-29T06:09:41Z

That's great. Thanks!

effigies reviewed Dec 17, 2018

View reviewed changes

effigies reviewed Dec 18, 2018

View reviewed changes

This was referenced Dec 21, 2018

FIX: Numpy pre-release accommodations #700

Merged

BUG: Fix deprecation warning on use of np.fromstring. #706

Closed

arokem added 7 commits December 26, 2018 13:18

RF: fromstring => frombuffer in externals.netcdf.

9caaed4

RF: fromstring => frombuffer in gifti/cifti2

bdac635

RF: fromstring => frombuffer.

ca19766

BF: Use a writeable bytearray for the header data.

c32e713

This is because newer numpy doesn't allow to change the writeable flag.

NF: Implement a readinto method for Opener.

d352dca

Remove unused variable

6924a56

arokem force-pushed the fromstring_deprecation branch from a696d0d to 6924a56 Compare December 26, 2018 21:18

effigies closed this in #700 Dec 31, 2018

effigies mentioned this pull request Apr 1, 2019

Cannot set values in GIFTI data array loaded from file #746

Closed

RF: Circumvents a deprecation warning from np.fromstring #702

RF: Circumvents a deprecation warning from np.fromstring #702

Uh oh!

Conversation

arokem commented Dec 17, 2018

Uh oh!

effigies commented Dec 17, 2018

Uh oh!

arokem commented Dec 17, 2018 via email

Uh oh!

coveralls commented Dec 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arokem commented Dec 17, 2018

Uh oh!

effigies commented Dec 17, 2018

Uh oh!

codecov-io commented Dec 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arokem commented Dec 17, 2018

Uh oh!

effigies left a comment

Choose a reason for hiding this comment

Uh oh!

effigies Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

arokem commented Dec 17, 2018 via email

Uh oh!

arokem commented Dec 17, 2018 via email

Uh oh!

effigies Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

arokem commented Dec 18, 2018 via email

Uh oh!

effigies commented Dec 18, 2018

Uh oh!

arokem commented Dec 18, 2018

Uh oh!

effigies commented Dec 18, 2018

Uh oh!

arokem commented Dec 18, 2018

Uh oh!

arokem commented Dec 26, 2018

Uh oh!

effigies commented Dec 27, 2018

Uh oh!

arokem commented Dec 29, 2018

Uh oh!

Uh oh!

RF: Circumvents a deprecation warning from `np.fromstring` #702

RF: Circumvents a deprecation warning from `np.fromstring` #702

coveralls commented Dec 17, 2018 •

edited

Loading

codecov-io commented Dec 17, 2018 •

edited

Loading