Closed danielballan closed 5 years ago
LGTM. It is more consistent now to use self.manager for both file open and close.
I will need to check if this actually works. There is a whole lot of state inside the tifffile.Tiffwriter obnject that needs to be cleaned up first, that is why in this case I did not use the manager.close() function
That would suggest to me that you would need additional code in close()
from what is there now, then. But having the manager close the files it opened vs having the serializer do the same should be equivalent.
So I just confirmed this does not actually work (by manual testing), if you have more than one image per file (say by generating a run using count(det, num=5) ) then only the first image is stored in the output file if closing using self.manager.close()
.
There is extra processes that occur in the tifffile.Tiffwriter
objects close call that cleans up the appending of the other images
To be clear this is a breaking change and should not be merged without a solution
Can you explain the difference? I'm not seeing it. I think the manager holds a reference to all the file handles, and so does the serializer. Either one could loop through those file handles and call file.close()
on each one. It does not matter where it's the serializer or the manager that does it.
the serializer holds a reference to the tifffile.Tiffwriter
objects not the file
objects that are associated with the manager. see this line.
closing the tiffile.TiffWriter
object performs extra cleanup not done by calling close on the file object.
Ah ha! Thanks for explaining. You are right.
no problem, I will put through a new PR later this afternoon with a better test that will catch this issue in the future !-)
I am happy with these changes now.
@licode Can you take one more look at this and merge?
LGTM. We need two steps on closing files.
Codecov Report
94.02% <100%> (+0.18%)
Continue to review full report at Codecov.