Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: io/netcdf: make mmap=False the default on PyPy #8483

Merged
merged 1 commit into from Mar 20, 2018

Conversation

@pv
Copy link
Member

commented Feb 25, 2018

On PyPy mmapped files usually can only be closed when the GC runs,
because variables referring to the mmap stay alive.

This is undesirable, so change the default value to mmap=False on pypy.
Also silence related warnings in tests.

ENH: io/netcdf: make mmap=False the default on PyPy
On PyPy mmapped files usually can only be closed when the GC runs,
because variables referring to the mmap stay alive.

This is undesirable, so change the default value to mmap=False on pypy.
Also silence related warnings in tests.
@tylerjereddy
Copy link
Contributor

left a comment

LGTM. Made one comment about switching to plain asserts at some stage.

"Cannot close a netcdf_file opened with mmap=True.*")
with open('simple.nc', 'rb') as fobj:
with netcdf_file(fobj, mmap=True) as f:
assert_(f.use_mmap)

This comment has been minimized.

Copy link
@tylerjereddy

tylerjereddy Feb 27, 2018

Contributor

Could maybe switch to plain assert now that we are using pytest, since pytest replaces plain assert with a bunch of useful stuff for error handling & prevents the removal of plain asserts by the python optimization flag. Conversely, not sure that we want a campaign of converting all the assert_ to assert anytime soon.

This comment has been minimized.

Copy link
@larsoner

larsoner Feb 27, 2018

Member

+1 for switching to assert statements in new code

@pv

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2018

@rgommers rgommers added this to the 1.1.0 milestone Mar 20, 2018

@rgommers rgommers merged commit d422b77 into scipy:master Mar 20, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 75.85% (target 1%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rgommers

This comment has been minimized.

Copy link
Member

commented Mar 20, 2018

LGTM too, in it goes. Does it make sense to backport this?

@pv

This comment has been minimized.

Copy link
Member Author

commented Mar 20, 2018

@rgommers

This comment has been minimized.

Copy link
Member

commented Mar 20, 2018

Probably not, unless you want the full Pypy-fix patch series

No, 1.1.0 should not be too far off, so let's leave it for that release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.