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

Rename pixel size property to match bioio-base #16

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

aswallace
Copy link
Contributor

@aswallace aswallace commented Jan 15, 2025

Link to Relevant Issue

This pull request resolves #14 : The physical_pixel_sizes property was not being updated on scene changes.
Requires the change in bioio-devs/bioio-base#27 for the property to actually reset

Description of Changes

Rename existing _px_sizes property to match _physical_pixel_sizes from bioio-base. This will make it so that when the property is reset from the base class (i.e., when the scene changes), pixel sizes will be recalculated in the LIF reader.

Testing

Verified that the existing tests still pass. Have not added a new unit test yet, but am open to doing so if needed.

Tested manually by reproducing the issue, as follows:

img_bio = BioImage(lif_path_str, reader=bioio_lif.Reader)

img_bio.set_scene(scene1_name)
print(f"scene 1: {img_bio.physical_pixel_sizes}")

img_bio.set_scene(scene2_name)
# pixel size previously would have remained the same, but now updates to the new pixel size
print(f"scene 2: {img_bio.physical_pixel_sizes}") 

@aswallace aswallace changed the title rename pixel size property to match bioio-base Rename pixel size property to match bioio-base Jan 15, 2025
Copy link

@evamaxfield evamaxfield left a comment

Choose a reason for hiding this comment

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

All tests pass and seems fine with me! Thanks!

@aswallace aswallace marked this pull request as ready for review January 15, 2025 22:11
@aswallace aswallace requested a review from a team as a code owner January 15, 2025 22:11
Copy link
Contributor

@BrianWhitneyAI BrianWhitneyAI left a comment

Choose a reason for hiding this comment

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

Nice!

@toloudis
Copy link

Is performance ok if you change scene, get pixel size, change scene, get pixel size, etc?

@aswallace
Copy link
Contributor Author

aswallace commented Jan 16, 2025

Is performance ok if you change scene, get pixel size, change scene, get pixel size, etc?

I did some extremely basic timeit tests on one of our multi-scene LIF files on Vast (~260 GB, 80 scenes) where I just did lif.set_scene(scene), print(lif.physical_pixel_sizes) multiple times and checked time in between. TBF, I'm not completely sure what factors impact time, so my choices may or may not make sense:

In case indexing matters, I switched between scenes that were not adjacent (e.g., 3 to 78 to 24). With the changes applied to bioio-lif and bioio-base, the first time a scene was accessed it took 1-30 secs to change to that new scene and get/print pixel size. When the code was re-run with the same scenes, it took less than a second to switch and get/print pixel size.

As a reference, for both versions of the code (with and w/o changes) it took me 5-18 minutes to run _read_immediate for the first time (which also re-calculates pixel size, though its time shouldn't be affected by these PRs), and then ~3 seconds to re-run if a scene had been accessed before

Worth noting that I'm testing this on vpn to access Vast files, so my times may not accurately reflect running on local files.

@toloudis
Copy link

toloudis commented Jan 16, 2025

We might just want a new issue ticket to see if we can/should make this faster. I don't have a good sense of how important LIF files will be moving forward within AICS.

@aswallace aswallace merged commit 8100780 into main Jan 17, 2025
13 checks passed
@aswallace aswallace deleted the bugfix/reset-pixel-size branch January 17, 2025 21:09
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.

physical_pixel_sizes is not not updated after changing scene
4 participants