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

Remove redundant array copy for TiffFileWSIReader #6089

Merged

Conversation

drbeh
Copy link
Member

@drbeh drbeh commented Mar 1, 2023

Fixes #5580

Description

This PR removes an "unnecessary" array copy in WSITiffFileReader, which was causing an slow down in loading whole slide images.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).

Signed-off-by: Behrooz <3968947+drbeh@users.noreply.github.com>
@drbeh drbeh requested review from wyli and Nic-Ma March 1, 2023 21:42
@drbeh drbeh enabled auto-merge (squash) March 1, 2023 21:43
@drbeh drbeh changed the title Remove redundant array copy for WSITiffFileReader Remove redundant array copy for TiffFileWSIReader Mar 1, 2023
@drbeh
Copy link
Member Author

drbeh commented Mar 1, 2023

from monai.data.wsi_reader import WSIReader
from monai.data.image_reader import WSIReader as WSIReader2

filename = "temp_CMU-1.tiff"
reader1 = WSIReader(backend="TiffFile")
reader2 = WSIReader2(backend="TiffFile")
obj1 = reader1.read(filename)
obj2 = reader2.read(filename)
%%timeit
reader1.get_data(obj1, level=1)

current performance: 2.32 s ± 97.7
after fixing the issue: 1.45 s ± 12.9 ms

%%timeit
reader2.get_data(obj2, level=1)

current performance: 1.97 s ± 50.2 ms
after fixing the issue:. 1.93 s ± 141 ms

@wyli
Copy link
Member

wyli commented Mar 1, 2023

thanks, is it possible to add a test to avoid regression?

@wyli
Copy link
Member

wyli commented Mar 1, 2023

/build

@drbeh
Copy link
Member Author

drbeh commented Mar 1, 2023

thanks, is it possible to add a test to avoid regression?

Since it was a performance regression, I am not sure how we should set the criteria for the test as it is hardware dependent, especially when dealing with variability across different run of the same class. Do you have any suggestion? Maybe we should just check for regression whenever we have a major change.

@wyli
Copy link
Member

wyli commented Mar 1, 2023

Maybe just check theres no array copy, which means changing the patch would also change the input array?

@drbeh
Copy link
Member Author

drbeh commented Mar 1, 2023

Maybe just check theres no array copy, which means changing the patch would also change the input array?

Right but in this case there is mo use for the input array since the array input is loaded from disk and is being prepared based on the arguments, so if we copy the original array, it will be discarded anyway.

@wyli
Copy link
Member

wyli commented Mar 2, 2023

Sure, there was a similar performance bug earlier #3322 hope we can add some tests to avoid reoccurring

Copy link
Member

@wyli wyli left a comment

Choose a reason for hiding this comment

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

would be great to have some integration tests

@wyli
Copy link
Member

wyli commented Mar 2, 2023

/build

@wyli
Copy link
Member

wyli commented Mar 2, 2023

/build

@drbeh drbeh merged commit 934ba09 into Project-MONAI:dev Mar 2, 2023
@drbeh drbeh deleted the fix-5580-wsireader-tifffile-performance branch March 2, 2023 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WSIReader, cucim , tifffile, performance regressions
2 participants