Added pathlib support for several functions #6660

wackywendell · 2015-11-10T02:09:16Z

Added support for pathlib.Path support for several functions, including savetxt, loadtxt, load, savez, savez_compressed, ndfromtxt, recfromtxt, and recfromcsv, along with testing. Fixes #6418.

Note that #6655 also addresses the same issue, and we should coordinate on this; I was a bit slow on making my changes, and @r0fls did some good work on his branch!

This could also use a bit of code review, I imagine.

CC @rgommers, @r0fls.

r0fls · 2015-11-10T02:50:00Z

numpy/lib/npyio.py

@@ -425,7 +429,7 @@ def save(file, arr, allow_pickle=True, fix_imports=True):

    Parameters
    ----------
-    file : file or str
+    file : file, str, or pathlib.Path
        File or filename to which the data is saved.  If file is a file-object,
        then the filename is unchanged.  If file is a string, a ``.npy``


You could update this to say If file is a string or a pathlib.Path object, a ''.npy'' extension will be appended to the file name if it does not already...

Good point, and done!

charris · 2015-11-15T19:53:14Z

numpy/lib/npyio.py

@@ -86,10 +86,12 @@ def __dir__(self):
        return object.__getattribute__(self, '_obj').keys()


-def zipfile_factory(*args, **kwargs):
+def zipfile_factory(file, mode='r', *args, **kwargs):


Umm, adding a positional argument seems like a potential compatibility problem.

I'm not sure how in this case it would be a problem. Could you be more specific?

Before, it was just passing *args to zipfile.Zipfile(); the first two args to zipfile.Zipfile() are necessarily file and mode, and AFAICT always have been. I'm not sure how this could be a backwards-compatibility problem.

Or are you thinking this might be a future compatibility problem?

By and large, signatures should not be changed without good reason, there is always the possibility that you have missed something nor can you rely on the tests being good enough to catch problems. This is particularly true for public functions, which unfortunately this is. The original was perhaps unfortunate, but there you go. It is best to keep PRs focused. If you feel this change is a useful cleanup, then it should come in as a separate PR.

Just to clarify the procedure ;) A new private function, say _zipfile_factory would be defined, and zipfile_factory would be deprecated. That may seem overly picky, but all such changes carry some risk, and when several hundred PRs are present in a release the risk accumulates.

OK, that makes sense. I'll work on it - probably by changing it back, because I don't think this is important enough to warrant a deprecation cycle.

I reverted the type signature, although I did add an assert. With the previous version, a 0-length *args would cause an error from zipfile.Zipfile; this just changes the exception that would be raised from a TypeError to an AssertionError. Is that ok, or should I also preserve the assertion type?

charris · 2015-11-15T20:54:18Z

See also #6655, which does much the same.

wackywendell · 2015-11-15T20:58:50Z

See also #6655, which does much the same.

Yes, #6655 adds pathlib.Path support to one function; this adds it to about 7. I could rebase this onto #6655, though, if you'd like... I was waiting to see if #6655 would be merged first, though.

charris · 2015-11-15T21:11:00Z

The PR list tends to act like a lifo :) I just saw #6655 as I was going down the list.

charris · 2015-11-16T02:47:01Z

numpy/lib/npyio.py

@@ -87,6 +87,13 @@ def __dir__(self):


 def zipfile_factory(*args, **kwargs):
+    """Create a zipfile.Zipfile. Allows for Zip64, as well as pathlib.Path 
+    objects."""
+    assert len(args) >= 1, "zipfile_factory requires a file"


I think you can keep the file argument, it isn't optional in the ZipFile constructor and you would get an error anyway without it.

class zipfile.ZipFile(file[, mode[, compression[, allowZip64]]])

Note that assert doesn't work when python is run in optimized mode. Just raise an error if you want to do it that way. Personally, I think a TypeError from ZipFile is fine, especially as it is unlikely to ever be seen.

Alright, now its zipfile_factory(file, *args, **kwargs), and the code is cleaner.

charris · 2015-11-16T21:41:06Z

numpy/lib/npyio.py

@@ -476,6 +482,11 @@ def save(file, arr, allow_pickle=True, fix_imports=True):
            file = file + '.npy'
        fid = open(file, "wb")
        own_fid = True
+    elif _is_pathlib_path(file):
+        if not file.name.endswith('.npy'):
+            file = file.parent / (file.name + '.npy')


Out of curiosity, what does the / do here. I haven't seen that before.

Ah, looked it up.

charris · 2015-11-16T22:04:57Z

I think we are almost there. Could you squash the commits and take a look at the commit message guidelines in doc/source/dev/gitwash/development_workflow.rst. Not sure what to do about the context manager for temporary files, as mentioned there is another PR with one. I'd like to have one available somewhere global, maybe in numpy/testing/utils.py

wackywendell · 2015-11-16T23:04:16Z

I think we are almost there. Could you squash the commits and take a look at the commit message guidelines

Thanks! Once we figure out the context manager thing, I'll squash / rebase / fix the commit messages.

rherault-pro · 2015-11-17T06:33:34Z

+++ to @charris we definitively need context managers for temp dirs and temp files that can be closed without removed in a larger context that tests.

rherault-pro · 2015-11-17T06:40:59Z

Moreover having functions accepting file descriptor and not only string or file path is a good move in the fact that they will be able to transparently accept in mem (temporary) files.

charris · 2015-12-20T17:59:38Z

I've committed a temppath context manager in #6866. If you look there, I also refactor a test to use it. You can get at it (and tempdir) by rebasing on master and importing from numpy.testing.

wackywendell · 2015-12-20T18:00:03Z

Great! I'll look into that.

charris · 2016-01-12T19:51:54Z

@wackywendell Could you finish this up and squash the commits into one with a rewritten commit message ala doc/source/dev/gitwash/development_workflow.rst?

wackywendell · 2016-01-12T21:02:10Z

Hi, sorry - I've been a bit side-tracked in the past month or two; I have a PhD defense in a month, and I'm going through interview stuff now, so this is taking me longer than I planned. I'll try and get it done as soon as I can, though.

njsmith · 2016-04-06T22:08:22Z

Only concern is that it sounds like python-dev is converging on a nicer way to do this that will probably be released in the next 3.4, 3.5 point releases. Maybe we don't care and can always clean this up later once that happens -- I don't have a strong opinion either way.

njsmith · 2016-04-06T22:09:21Z

Probably we should just go for it given that the PR exists :-). I won't merge myself though b/c I'm just thinking about the high-level issue, I haven't actually read the code.

charris · 2016-04-06T23:16:28Z

numpy/compat/py3k.py

@@ -10,6 +10,10 @@
           'integer_types']



Add Path and is_pathlib_path to the __all__ list, then you can import from numpy.compat.

charris · 2016-04-06T23:23:33Z

I think we are getting close. @njsmith Should be easy to search and fixup. What changes are being proposed?

EDIT: Is pathlib going away?

njsmith · 2016-04-06T23:30:42Z

@charris: it looks like the current trend is towards adding a standard convert-to-filesystem-path protocol sorta along the lines of __index__, so APIs that take paths and want strings would do from operators import fspath and then

def some_file_function(path, ...):
    # force path to be a str
    path = fspath(path)
    ...

Or something like that.

Also probably pathlib support will get added to a bunch of builtins like open, so that for most code no special handling will be needed at all -- you just write

def some_file_function(path, ...):
    with open(path) as f:
        ...

and it just magically works for both str and Path arguments.

njsmith · 2016-04-06T23:31:55Z

But, it's a mailing list discussion, so -- that's where things seem to be going right now, but, check back next week for our thrilling conclusion :-).

charris · 2016-04-07T01:40:41Z

numpy/lib/tests/test_io.py

+
+class TestPathUsage(TestCase):
+    # Test that pathlib.Path can be used
+    @np.testing.dec.skipif(Path is None,


Umm, this will now fit on a single line. Same below.

charris · 2016-04-07T01:42:37Z

Looks good modulo two style nits.

charris · 2016-04-07T13:06:35Z

Thanks @wackywendell .

ChrisBarker-NOAA · 2016-04-08T16:16:38Z

Sorry to not look at the code -- but is this going to work in py2 with the pypi pathlib2 object? If so -- great!

charris · 2016-04-08T16:52:29Z

No, it won't work with pathlib2 because the module name differs. @wackywendell That could probably be fixed.

ChrisBarker-NOAA · 2016-04-08T17:00:28Z

well, there is a new protocol being proposed -- something like a fspath dunder method. So maybe just wait 'till that gets finalized.

charris · 2016-04-08T17:23:25Z

@ChrisBarker-NOAA Probably a good idea. There is also tofile and fromfile to worry about, but they are at the C level and so it will be worth waiting to see what the final interface is.

eric-wieser · 2017-02-09T11:46:38Z

That protocol has been finalized in PEP 519, and is live in python 3.6

wackywendell force-pushed the usepath branch 2 times, most recently from a117f3a to 83c0513 Compare November 10, 2015 02:41

r0fls reviewed Nov 10, 2015
View reviewed changes

wackywendell force-pushed the usepath branch from 83c0513 to b7ea047 Compare November 10, 2015 17:49

charris added 01 - Enhancement component: numpy.lib labels Nov 11, 2015

charris reviewed Nov 15, 2015
View reviewed changes

charris mentioned this pull request Nov 15, 2015

Allow pathlib.Path arguments, from issue #6418 #6655

Closed

wackywendell force-pushed the usepath branch from d8da42a to 0aac04d Compare November 16, 2015 02:33

charris reviewed Nov 16, 2015
View reviewed changes

wackywendell force-pushed the usepath branch from 0aac04d to 7622fa9 Compare November 16, 2015 02:56

charris reviewed Nov 16, 2015
View reviewed changes

charris mentioned this pull request Nov 17, 2015

ENH: switch to 'with' statement for open/close file in lib.npyio._savez #6545

Closed

charris reviewed Apr 6, 2016
View reviewed changes

wackywendell force-pushed the usepath branch from 88522fd to bf5267d Compare April 6, 2016 23:34

charris reviewed Apr 7, 2016
View reviewed changes

wackywendell force-pushed the usepath branch from bf5267d to da29693 Compare April 7, 2016 02:16

ENH: Add support for pathlib.Path objects to save/load functions

5ac270b

wackywendell force-pushed the usepath branch from da29693 to 5ac270b Compare April 7, 2016 02:17

charris merged commit 0c6aa60 into numpy:master Apr 7, 2016

homu mentioned this pull request Apr 7, 2016

WIP: implement __rop__ logic for scalar operators #7459

Closed

anntzer mentioned this pull request Feb 7, 2017

fromfile could accept pathlib.Path argument #8576

Closed

paulmueller mentioned this pull request Jun 15, 2018

load does not accept pathlib.Path in mmap_mode #11342

Closed

anntzer mentioned this pull request Nov 9, 2019

Bump minimal numpy version to 1.12. matplotlib/matplotlib#15645

Merged

6 tasks

mpetroff mentioned this pull request Nov 15, 2019

Allow pathlib.Path paths for loading models via Keras API tensorflow/tensorflow#34254

Closed

Uh oh!

Added pathlib support for several functions #6660

Added pathlib support for several functions #6660

Uh oh!

Conversation

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!