BUG: fix scipy.io.mmread() of gzipped files under Python3 #3314

Merged
merged 2 commits into from Feb 24, 2014

Conversation

Projects
None yet
5 participants
@andreas-h
Contributor

andreas-h commented Feb 12, 2014

fixes #2152

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Feb 12, 2014

Coverage Status

Changes Unknown when pulling 8daf621 on andreas-h:gh2152 into * on scipy:master*.

Coverage Status

Changes Unknown when pulling 8daf621 on andreas-h:gh2152 into * on scipy:master*.

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Feb 12, 2014

Coverage Status

Changes Unknown when pulling 8daf621 on andreas-h:gh2152 into * on scipy:master*.

Coverage Status

Changes Unknown when pulling 8daf621 on andreas-h:gh2152 into * on scipy:master*.

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Feb 12, 2014

Coverage Status

Changes Unknown when pulling 8daf621 on andreas-h:gh2152 into * on scipy:master*.

Coverage Status

Changes Unknown when pulling 8daf621 on andreas-h:gh2152 into * on scipy:master*.

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Feb 12, 2014

Coverage Status

Changes Unknown when pulling 8daf621 on andreas-h:gh2152 into * on scipy:master*.

Coverage Status

Changes Unknown when pulling 8daf621 on andreas-h:gh2152 into * on scipy:master*.

scipy/io/mmio.py
@@ -451,9 +452,14 @@ def _parse_body(self, stream):
return coo_matrix((rows, cols), dtype=dtype)
try:
+ # passing a gzipped file to fromfile/fromstring doesn't work
+ # with Python3
+ if sys.version_info >= (3, 0) and isinstance(stream, gzip.GzipFile):

This comment has been minimized.

Show comment Hide comment
@juliantaylor

juliantaylor Feb 12, 2014

Contributor

probably the same occurs with bz2, zip and xz files

@juliantaylor

juliantaylor Feb 12, 2014

Contributor

probably the same occurs with bz2, zip and xz files

This comment has been minimized.

Show comment Hide comment
@juliantaylor

juliantaylor Feb 12, 2014

Contributor

numpy uses np.compat.isfileobj to determine if fromfile can be used

@juliantaylor

juliantaylor Feb 12, 2014

Contributor

numpy uses np.compat.isfileobj to determine if fromfile can be used

This comment has been minimized.

Show comment Hide comment
@juliantaylor

juliantaylor Feb 12, 2014

Contributor

on second thought that function is too restrictive it rejects io based file objects which are default in python3

@juliantaylor

juliantaylor Feb 12, 2014

Contributor

on second thought that function is too restrictive it rejects io based file objects which are default in python3

This comment has been minimized.

Show comment Hide comment
@juliantaylor

juliantaylor Feb 12, 2014

Contributor

ignore the last comment, I opened files in write modes, it seems to work if they are binary read mode.

@juliantaylor

juliantaylor Feb 12, 2014

Contributor

ignore the last comment, I opened files in write modes, it seems to work if they are binary read mode.

@andreas-h

This comment has been minimized.

Show comment Hide comment
@andreas-h

andreas-h Feb 18, 2014

Contributor

added checking for bzip2 files. couldn't find xz support in mmread at all.

Contributor

andreas-h commented Feb 18, 2014

added checking for bzip2 files. couldn't find xz support in mmread at all.

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Feb 18, 2014

Coverage Status

Coverage remained the same when pulling e634d59 on andreas-h:gh2152 into 7cefb25 on scipy:master.

Coverage Status

Coverage remained the same when pulling e634d59 on andreas-h:gh2152 into 7cefb25 on scipy:master.

@pv

This comment has been minimized.

Show comment Hide comment
@pv

pv Feb 19, 2014

Member

Test?

Member

pv commented Feb 19, 2014

Test?

@andreas-h

This comment has been minimized.

Show comment Hide comment
@andreas-h

andreas-h Feb 19, 2014

Contributor

can we assume that gzip and bz2 modules are available on all platforms? if not, how can I mark the tests accordingly?

Contributor

andreas-h commented Feb 19, 2014

can we assume that gzip and bz2 modules are available on all platforms? if not, how can I mark the tests accordingly?

@andreas-h

This comment has been minimized.

Show comment Hide comment
@andreas-h

andreas-h Feb 19, 2014

Contributor

I just saw that Py3.3 has a lzma module. Should we add support for .xz files to mmread, which would then only be available on Python >= 3.3?

Contributor

andreas-h commented Feb 19, 2014

I just saw that Py3.3 has a lzma module. Should we add support for .xz files to mmread, which would then only be available on Python >= 3.3?

@pv

This comment has been minimized.

Show comment Hide comment
@pv

pv Feb 19, 2014

Member

AFAIK at least the bz2 module is not necessarily present in all Python builds.
Maybe you can try-expect guard the imports?

Member

pv commented Feb 19, 2014

AFAIK at least the bz2 module is not necessarily present in all Python builds.
Maybe you can try-expect guard the imports?

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Feb 19, 2014

Coverage Status

Coverage remained the same when pulling 14cb83d on andreas-h:gh2152 into 7cefb25 on scipy:master.

Coverage Status

Coverage remained the same when pulling 14cb83d on andreas-h:gh2152 into 7cefb25 on scipy:master.

@pv pv added PR labels Feb 19, 2014

@andreas-h

This comment has been minimized.

Show comment Hide comment
@andreas-h

andreas-h Feb 20, 2014

Contributor

The BZ2File and GzipFile classes don't support with statements in Python2.6. Fixed that now.

Contributor

andreas-h commented Feb 20, 2014

The BZ2File and GzipFile classes don't support with statements in Python2.6. Fixed that now.

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Feb 20, 2014

Coverage Status

Coverage remained the same when pulling ac31bd8 on andreas-h:gh2152 into c08dd62 on scipy:master.

Coverage Status

Coverage remained the same when pulling ac31bd8 on andreas-h:gh2152 into c08dd62 on scipy:master.

@rgommers rgommers added scipy.io and removed needs-work labels Feb 23, 2014

scipy/io/tests/test_mmio.py
+ def test_bzip2_py3(self):
+ # test if fix for #2152 works
+ try:
+ import bz2

This comment has been minimized.

Show comment Hide comment
@rgommers

rgommers Feb 23, 2014

Member

this needs a comment on when the module is/isn't available

@rgommers

rgommers Feb 23, 2014

Member

this needs a comment on when the module is/isn't available

This comment has been minimized.

Show comment Hide comment
@andreas-h

andreas-h Feb 23, 2014

Contributor

Can you be more specific? I'm not sure how such things are handled in
the tests.

@andreas-h

andreas-h Feb 23, 2014

Contributor

Can you be more specific? I'm not sure how such things are handled in
the tests.

This comment has been minimized.

Show comment Hide comment
@rgommers

rgommers Feb 23, 2014

Member

Just above the import I would just put # bz2 module isn't always built when building Python. Otherwise one always wonders why the import is guarded. The stdlib docs at http://docs.python.org/2/library/bz2.html don't even explain this:(

@rgommers

rgommers Feb 23, 2014

Member

Just above the import I would just put # bz2 module isn't always built when building Python. Otherwise one always wonders why the import is guarded. The stdlib docs at http://docs.python.org/2/library/bz2.html don't even explain this:(

This comment has been minimized.

Show comment Hide comment
@andreas-h

andreas-h Feb 23, 2014

Contributor

Shall I add the same note to the gzip import? Or is gzip always present
and I can remove the try/except?

@andreas-h

andreas-h Feb 23, 2014

Contributor

Shall I add the same note to the gzip import? Or is gzip always present
and I can remove the try/except?

This comment has been minimized.

Show comment Hide comment
@rgommers

rgommers Feb 23, 2014

Member

No, gzip can also be missing. Adding same note makes sense.

@rgommers

rgommers Feb 23, 2014

Member

No, gzip can also be missing. Adding same note makes sense.

This comment has been minimized.

Show comment Hide comment
@andreas-h

andreas-h Feb 23, 2014

Contributor

Done.

@andreas-h

andreas-h Feb 23, 2014

Contributor

Done.

@rgommers

This comment has been minimized.

Show comment Hide comment
@rgommers

rgommers Feb 23, 2014

Member

removing needs-work label, looks like previous comments were addressed.

Member

rgommers commented Feb 23, 2014

removing needs-work label, looks like previous comments were addressed.

@rgommers rgommers added this to the 0.14.0 milestone Feb 23, 2014

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Feb 23, 2014

Coverage Status

Coverage remained the same when pulling 541dcc4 on andreas-h:gh2152 into 0da153e on scipy:master.

Coverage Status

Coverage remained the same when pulling 541dcc4 on andreas-h:gh2152 into 0da153e on scipy:master.

rgommers added a commit that referenced this pull request Feb 24, 2014

Merge pull request #3314 from andreas-h/gh2152
BUG: fix scipy.io.mmread() of gzipped files under Python3

@rgommers rgommers merged commit 46d7a3f into scipy:master Feb 24, 2014

1 check passed

default The Travis CI build passed
Details
@rgommers

This comment has been minimized.

Show comment Hide comment
@rgommers

rgommers Feb 24, 2014

Member

Merging, looks fine now. Thanks @andreas-h

Member

rgommers commented Feb 24, 2014

Merging, looks fine now. Thanks @andreas-h

@pv

This comment has been minimized.

Show comment Hide comment
@pv

pv Feb 24, 2014

Member

One remaining buglet fixed in 86d3722 (pushed to master).

I don't really like checking for "bad classes" manually, but on the other hand, I'm not sure if it's even possible to do this checking in a generic way.

Member

pv commented Feb 24, 2014

One remaining buglet fixed in 86d3722 (pushed to master).

I don't really like checking for "bad classes" manually, but on the other hand, I'm not sure if it's even possible to do this checking in a generic way.

@rgommers

This comment has been minimized.

Show comment Hide comment
@rgommers

rgommers Feb 24, 2014

Member

Thanks

Member

rgommers commented Feb 24, 2014

Thanks

@andreas-h andreas-h deleted the andreas-h:gh2152 branch Jun 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment