search

NKI-AI / dlup

Dlup are the Deep Learning Utilities for Pathology developed at the Netherlands Cancer Institute
Apache License 2.0
25 stars 7 forks source link

Properly close open file handlers with PYVIPS image backend #238

Closed AjeyPaiK closed 4 months ago

AjeyPaiK commented 4 months ago

Fixes #237

BPdeRooij commented 4 months ago

The PR looks good to me. Properly closing the self._image in the PyVipsSlide backend fixes the issue. The PyVipsSlide does not have proper tests in tests/test_backend.py. If we want PyVipsSlide as the main backend for dlup, the tests should be similar to the tests on OpenSlideSlide and TifffileSlide. I do recall that there were some issues with testing of the PyVipsSlide backend before. It would be interesting to discuss here and to hear your input.

jonasteuwen commented 4 months ago

There is a 73% coverage for the pyvips backend through other tests. There was a difficulty testing because the tests were still written for OpenSlide. Now this has been improved. To get 100% test coverage we would need to monkey-patch some parts. The major important parts are tested.

I would propose to keep on using it until we run into more mistakes, and eventually go to a test coverage of 100% to release v1.0. I have some more suggestions in terms of input/output formats for annotations as well.

AjeyPaiK commented 4 months ago

Thanks for the review Bart