TissueImageAnalytics / tiatoolbox

Computational Pathology Toolbox developed by TIA Centre, University of Warwick.
https://warwick.ac.uk/tia
Other
375 stars 77 forks source link

🐛 Fix `TIFFWSIReader` `read_bound` #777

Closed vqdang closed 3 months ago

vqdang commented 8 months ago

image

Emergency bugfix per @John-P request. The culprit is reading bound doesn't use the adjusted bounds as have been done in OpenSlideReader.

shaneahmed commented 8 months ago

Thanks @vqdang and @John-P Please can you add a simple test to make sure this produces expected results.

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.86%. Comparing base (621a857) to head (a2c8afe). Report is 1 commits behind head on develop.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #777 +/- ## =========================================== - Coverage 99.89% 99.86% -0.04% =========================================== Files 69 69 Lines 8650 8650 Branches 1653 1654 +1 =========================================== - Hits 8641 8638 -3 - Misses 1 4 +3 Partials 8 8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

shaneahmed commented 8 months ago

Please can you also check if it is related to #499 ?

Abdol commented 4 months ago

I have added a simple test but not completely sure if it fully tests for the fixed bug. @vqdang @John-P I'd appreciate it if you can give the test a review.

Abdol commented 3 months ago

A kind follow up @vqdang @John-P. I'd appreciate it if you can give the test a review. Thanks!

measty commented 3 months ago

I think this also needs a fix along similar lines for read_rect

Abdol commented 3 months ago

A kind follow up @vqdang @John-P. I'd appreciate it if you can give the test a review. Thank you!

measty commented 3 months ago

I've added a few changes to this PR too.

Abdol commented 3 months ago

After updating the sample OME TIFF, the bug can be reproduced using the existing level consistency tests. So, I have removed the newly-added test. This PR should be good to go now.