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

Add MicroManagerTiffImagingExtractor #222

Merged
merged 24 commits into from
May 2, 2023

Conversation

weiglszonja
Copy link
Contributor

@weiglszonja weiglszonja commented Apr 21, 2023

Add extractor for the Micro-Manager TIF image format:

  • image file stacks are saved into multipage TIF files in OME-TIFF format (.ome.tif files), each of which are up to around 4GB in size.
  • The 'DisplaySettings' JSON file contains the properties of Micro-Manager.

@weiglszonja weiglszonja marked this pull request as draft April 21, 2023 16:33
@weiglszonja weiglszonja self-assigned this Apr 24, 2023
@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review May 2, 2023 14:23
@CodyCBakerPhD CodyCBakerPhD self-requested a review May 2, 2023 14:23
video_shape = (stop - start,) + self._imaging_extractors[0].get_image_size()
video_shape = (stop - start,) + self.get_image_size()
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to change this?

Since it's an abstract method wouldn't you need to also define get_image_size generally for the MultiImagingExtractor?

I'd prefer if you can leave the base class here unchanged and do what you need to in the child classes, is that possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, it's not necessary to change, better to keep it as is.

@CodyCBakerPhD
Copy link
Member

Overall looks good, one comment that has the potential to break back-compatibility (though I admit it's likely not being tested explicitly)

@codecov
Copy link

codecov bot commented May 2, 2023

Codecov Report

Merging #222 (3763330) into master (4494155) will increase coverage by 1.17%.
The diff coverage is 99.15%.

❗ Current head 3763330 differs from pull request most recent head ab63342. Consider uploading reports for the commit ab63342 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
+ Coverage   74.27%   75.45%   +1.17%     
==========================================
  Files          34       35       +1     
  Lines        2395     2514     +119     
==========================================
+ Hits         1779     1897     +118     
- Misses        616      617       +1     
Flag Coverage Δ
unittests 75.45% <99.15%> (+1.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/roiextractors/extractorlist.py 100.00% <ø> (ø)
...gingextractors/micromanagertiffimagingextractor.py 99.15% <99.15%> (ø)
...ctors/extractors/tiffimagingextractors/__init__.py 100.00% <100.00%> (ø)

@CodyCBakerPhD CodyCBakerPhD merged commit 4195e63 into master May 2, 2023
@CodyCBakerPhD CodyCBakerPhD deleted the add_MicroManagerTiffImagingExtractor branch May 2, 2023 19:10
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.

2 participants