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

MikroscanTiffReader Addition #3333

Open
wants to merge 4 commits into
base: develop
from

Conversation

Projects
None yet
4 participants
@redcrow
Copy link

commented Mar 25, 2019

Submitting a new reader for Mikroscan Tiff files. Mikroscan is a provider of Digital Pathology and Telemicroscopy Solutions.

redcrow added some commits Mar 21, 2019

MikroscanTiffReader
Added MikroscanTiffReader to read Mikroscan TIF files
Update MikroscanTiffReader.java
Cleaned up some items
@dgault

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Hi @redcrow, thank you for taking the time to contribute and submitting a new reader.

Is there a public specification for the Mikroscan format or one which you would be able to share privately? Likewise are there any sample files which could be shared (ideally publicly but if not privately is fine) for testing purposes? For each reader we keep a repository of sample files which we test against every day in order to ensure no regressions in the format.

@redcrow

This comment has been minimized.

Copy link
Author

commented Mar 28, 2019

@dgault

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Thanks @redcrow, and apologies for the delayed response. If you are able to upload them to https://www.openmicroscopy.org/qa2/qa/upload/ that would be perfect.

@redcrow

This comment has been minimized.

Copy link
Author

commented Apr 3, 2019

@sbesson

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Thanks @redcrow, we received all your files and will take a look at them shortly.

@melissalinkert

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

Thanks again for putting this together, @redcrow.

In reviewing this briefly, my main concern is that much of the reader code duplicates what is in SVSReader. This makes complete sense based upon the specification, but I think it would be cleaner to have MikroscanTiffReader extend SVSReader, with any common code removed from MikroscanTiffReader.

The only other immediate issue I see is that the thumbnail IFD (IFD 1) may need to be handled differently. Right now, it is added as the last resolution in the image pyramid, but the image that is read is mostly blank (showinf -noflat -resolution 4 with either file will show this). The missing pixel data appears to result from a mismatch between the number of strips stored (one per row of pixels) and value of the RowsPerStrip tag (equal to the image height). Typically RowsPerStrip is set to 1 when the number of strips matches the image height, see https://www.awaresystems.be/imaging/tiff/tifftags/rowsperstrip.html

@redcrow

This comment has been minimized.

Copy link
Author

commented Apr 4, 2019

@redcrow

This comment has been minimized.

Copy link
Author

commented Apr 5, 2019

@melissalinkert

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

The easiest thing to do is just add the corresponding constructor to SVSReader. If you add public SVSReader(String name, String[] suffixes) that just calls super(name, suffixes) (exactly like https://github.com/openmicroscopy/bioformats/blob/develop/components/formats-bsd/src/loci/formats/in/BaseTiffReader.java#L89), then that should work.

@redcrow

This comment has been minimized.

Copy link
Author

commented Apr 8, 2019

If there are no issues with adding the needed constructor to the SVSReader.java class I will go that route. Thanks

MikroscanTiffReader Addition - Extend SVSReader
Based on feedback updated the MikroscanTiffReader class to inherit from SVSReader. Added a new constructor to SVSReader.

@dgault dgault added the reader:new label Apr 11, 2019

@redcrow

This comment has been minimized.

Copy link
Author

commented Apr 11, 2019

Uploaded new images that match between the number of strips stored (one per row of pixels) and value of the RowsPerStrip tag.

  1. MikroscanTiff_abnormalkidney.tif
  2. MikroscanTiff_appendix.tif
@dgault

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Thank you for uploading the new sample files, I can confirm we have received them successfully

@redcrow

This comment has been minimized.

Copy link
Author

commented Apr 17, 2019

Is there more to be done for this pull request or can it be closed?

@dgault dgault added the include label Apr 22, 2019

@dgault

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

I have pushed an update for the 'isThisType' method which will now correctly allow Mikroscan files to be identified based on Mikroscan image description prefeix. Previously this was being skipped and other Tiff formats were being correctly identified as Mikroscan.

This does highlight the fact that the same logic in the SVSReader is broken and we will need to update it. Currently we do not see the problems with it highlighted due to the different suffix used for SVS.

Also from testing all of the example files (which are now configured) open and display as expected with the exception of MikroscanTiff-lymph.tifwhich seems to display incorrect images for the thumbnails and the last 3 series. The first 4 series display correctly. As the functionality for this is contained with the SVSReader, it looks like there may be follow up fixes which we will need to make in the SVSReader.

@redcrow

This comment has been minimized.

Copy link
Author

commented Apr 23, 2019

The initial whole slide file images submitted MikroscanTiff-kidney.tif and MikroscanTiff-lymph.tif had an issue that was brought up by @melissalinkert (mismatch between the number of strips stored (one per row of pixels) and value of the RowsPerStrip tag). I fixed this in our output and it is reflected in the last two images submitted (MikroscanTiff_abnormalkidney.tif and MikroscanTiff_appendix.tif).

The first two images are no longer relevant and need to be removed from the example files.

@redcrow

This comment has been minimized.

Copy link
Author

commented May 14, 2019

@dgault Can this be closed at this point?

@sbesson sbesson added this to the 6.2.0 milestone May 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.