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 reader for PicoQuant .bin files. #1245
Conversation
Code reviewed and tested with On the code side, the only question I have is the hardcoding of the Z and C dimension sizes as 1. Is this file format technically capable of storing addtional Z planes or channels? If it can, openBytes will need to cache the series no and invalidate its cache if Z/C change. Also, is the pixelType always XYTCZ? If Z and C never vary, this doesn't matter. Other than that question, it all looks fine as far as I can tell. There's a couple of comments which could use reindenting and a few extra trailing spaces [e.g. at |
Re: hard coding of sizeC & sizeZ PicoQuant provided us with example code for reading this format (in Pascal) & this has no capability of handling more than x,y & t. The header in the example file I have available also has no space to store info about other dimensions. |
Cosmetic changes if desired: Indents: lines 82, 123, 147, 149 |
@melissalinkert Please could you copy the content of |
CoreMetadata m = core.get(0); | ||
|
||
m.littleEndian = true; | ||
in.order(isLittleEndian()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to have multiple calls to in.order
.
Some mostly minor comments above, but otherwise I think this is fine. QA 9434 has been copied to |
…into develop-PQBin
…loaded to obviate future memory issues. NB loading is 4-5 times slower.
Having finally, with much assistance, managed to get bio-formats to build locally again I think the memory issues have now been addressed. |
|
||
// disable pre-load mode for very large files | ||
// threshold is set to the size of the largest test file currently available | ||
if ( m.sizeX * m.sizeY * m.sizeT > (1288 * 200 * 200)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be set to slightly smaller than the largest file? That way we're sure to be testing both openBytes code paths. 128MB of pixels (so check 32 * 1024 * 1024
) would probably be reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I have only one file to test against & from what rleigh said you have the same test file in which case we can only test one code path. I have tested with a lower threshold locally. Let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we have two test files - one is 98MB, and one is 197MB.
Threshold set as requested. Please note this means that larger files will, typically, take very much longer to load. |
All seems fine now, thanks. |
Add reader for PicoQuant .bin files.
Hi, let me know if you need any additional bin files for testing. |
@snizzleorg I think that more test files would be very useful. Thanks. |
@melissalinkert Are you happy for this to be rebased to 5.0 ? |
@imunro: yes, go ahead. |
--rebased-to #1287 |
This adds support for one of the formats covered by https://trac.openmicroscopy.org.uk/ome/ticket/8059#
NB I am led to believe that the other format is a FIFO rather than imaging format in which case it could prove 'challenging'.