Skip to content
This repository was archived by the owner on Dec 1, 2025. It is now read-only.

fix imread bug and allow AICSImage class to close its reader#44

Merged
toloudis merged 7 commits into
masterfrom
fix/aicsimage-close
Oct 31, 2019
Merged

fix imread bug and allow AICSImage class to close its reader#44
toloudis merged 7 commits into
masterfrom
fix/aicsimage-close

Conversation

@toloudis

Copy link
Copy Markdown
Collaborator

The fixes a bug with imread which was leaving the file handle open. It also allows the with AICSImage as varname syntax, and adds a public method to close the reader.

Comment thread aicsimageio/tests/test_aics_image.py Outdated
with pytest.raises(IOError):
img = AICSImage(tempfilename)
img.metadata
os.remove(tempfilename)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked this up. It won't ever raise on Unix.
If a Filepointer is open on *nix it has translated the name to a pointer.
if you remove the file after this step it will nuke the file name but it the on-disk space pointed to by the pointer isn't closed until there are no pointer references into the file.
Windows is fundamentally different.

@codecov-io

codecov-io commented Oct 31, 2019

Copy link
Copy Markdown

Codecov Report

Merging #44 into master will increase coverage by 0.16%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
+ Coverage   87.96%   88.12%   +0.16%     
==========================================
  Files          27       27              
  Lines        1113     1128      +15     
==========================================
+ Hits          979      994      +15     
  Misses        134      134
Impacted Files Coverage Δ
aicsimageio/aics_image.py 96.15% <100%> (+0.43%) ⬆️
aicsimageio/tests/test_aics_image.py 100% <100%> (ø) ⬆️

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 45d93f4...823795c. Read the comment docs.

@heeler heeler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, clever solution using _bytes rather than querying the filesystem

@evamaxfield evamaxfield left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a part of this can you update the quick start in the README as well?

)
def test_aicsimage_close(resources_dir, filename):
img = AICSImage(resources_dir / filename)
assert img.reader._bytes.closed is False

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The """pythonic""" way to do this is assert not img.reader._bytes.closed 🙃

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I like this better tbh


with AICSImage(resources_dir / filename) as img:
img.metadata
assert img.reader._bytes.closed is True

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should definitely just be assert img.reader._bytes.closed

@toloudis toloudis merged commit 2af5803 into master Oct 31, 2019
@evamaxfield evamaxfield deleted the fix/aicsimage-close branch October 31, 2019 23:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants