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

Andor SIF: Support files larger than 4 GB and with non-fixed-size footers #4195

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

WalkerKnapp
Copy link

Hello! Our lab generates a large number of Andor SIF files which run into issues being loaded using bioformats (a IllegalArgumentException is thrown upon reading the first frame). As it turns out, this is the result of two issues:

  • Our files contain more than 4 gb of pixel data, causing an integer overflow when calculating the offset in the file for pixel information (see commit 0f36dd6)
  • Our files seem to have a section of XML appended to the end of them when exported from Andor Solis. See:
Output of tail for one of our Andor SIF files

D�D�D�CD�DD��C� D�C��C��C@      D�D��C�C��C�C@  DD@D@D��C��C��C�DD��C��C��C��C�C�D      DDD�C�C��C�CD�D��C�D@
                                                                                                             DD�D@D�D�D@D��CD�D��CD�D��C@D��C�C��C�D�C�D��C�D�C
                                                 D��CD�D�D@D�D�C��C�C��C�D�C�C�CD�D@D�D�C�C�C�D��C�C0
0
0
0
<?xml version="1.0" ?>
<Signals>
    <Signal>
        <TInstaImageData>
            <PreAmpGainText>Gain 1</PreAmpGainText>
            <SpectrographSerial></SpectrographSerial>
            <mSec>381</mSec>
            <DIRECT_IRIS_PRESENT>0</DIRECT_IRIS_PRESENT>
            <DIRECT_IRIS_POSITION>-1</DIRECT_IRIS_POSITION>
            <SIDE_IRIS_PRESENT>0</SIDE_IRIS_PRESENT>
            <SIDE_IRIS_POSITION>-1</SIDE_IRIS_POSITION>
            <FullSerialNumber>X-14188</FullSerialNumber>
            <DARKTHRESHOLD>-999</DARKTHRESHOLD>
            <FAST_KINETICS_OFFSET>0</FAST_KINETICS_OFFSET>
        </TInstaImageData>
        <TInstaImage>
            <IRIGAvailable>0</IRIGAvailable>
        </TInstaImage>
        <TCalibImage>
            <TCalibImageData />
            <TAndorImage>
                <TAndorImageData>
                    <ImageFormat>0</ImageFormat>
                    <ReadoutPort>-1</ReadoutPort>
                    <ExposureWindowHeight>0</ExposureWindowHeight>
                    <LineScanSpeed>0</LineScanSpeed>
                    <AlternatingReadoutDirection>0</AlternatingReadoutDirection>
                    <ScanSpeedControl>0</ScanSpeedControl>
                    <ReadoutDirection>-1</ReadoutDirection>
                    <FastKineticsStorageMode>0</FastKineticsStorageMode>
                    <FastKineticsTimeScanMode>-1</FastKineticsTimeScanMode>
                    <TickInterval>10</TickInterval>
                    <PIVMode>0</PIVMode>
                    <ExtendedDynamicRange>0</ExtendedDynamicRange>
                </TAndorImageData>
            </TAndorImage>
        </TCalibImage>
    </Signal>
</Signals>
<References />
<Backgrounds />
<Lives />
<Sources />
�SIFX%  

Both of these issues can be solved by parsing the entire SIF header, instead of relying on the positioning of the footer to determine where pixel data starts. I modified the header parsing based on @fujiisoup's sif_parser (https://github.com/fujiisoup/sif_parser/blob/a922fc299b749057f66bedaba3b6971e18f94c4e/sif_parser/_sif_open.py), which is able to open our files without issue.

Opening as a draft for now, because I am not sure if this works with all SIF file versions. Feedback and others testing their SIF files would be appreciated!

Specifically, on line 163, I believe parsing will fail when the first two bytes of image data are 0x300A. As far as I understand, this would only happen if the top-left pixel on frame 0 contains a value on the order of 5e-10. I can't think of a scenario where you would have measurements that small, but I would be interested to see if others could come up with one!

@WalkerKnapp WalkerKnapp changed the title Andor SIF: Support files larger than 4 GB and with non-standard footers Andor SIF: Support files larger than 4 GB and with non-fixed-size footers Jun 7, 2024
@dgault
Copy link
Member

dgault commented Jun 10, 2024

Thanks @WalkerKnapp for opening the initial draft. If are able to provide some sample files demonstrating the issue that would greatly help the testing. If you need a suitable upload location then we recommend using https://zenodo.org/.

Also, we have a Contributor License Agreement for the project, would you be able to sign and return the form following the instructions on https://ome-contributing.readthedocs.io/en/latest/cla.html

The first step for testing and review will be to move this PR out draft status and we will then include it as part of the CI and our daily test suite. This will test for any regressions in existing sample files that we have.

@dgault dgault marked this pull request as ready for review June 10, 2024 14:12
@dgault dgault added the include label Jun 10, 2024
@WalkerKnapp
Copy link
Author

Completed a CLA! In regards to publicly hosting a sample file, I will work on getting that approved (there are questions of rights and funding that are above my pay grade). In the meantime, I can also share example files from our university's cluster to anyone interested who has a Globus account, if that would be helpful.

@dgault
Copy link
Member

dgault commented Jun 11, 2024

Thanks @WalkerKnapp, I can confirm that we have received your CLA. There was only a single failure from the nightly tests, and I suspect that may actually be a fix that requires us to update our configs. Unfortunately that file isn't public, but I will investigate and confirm if that is the case.

@dgault dgault removed the include label Jun 12, 2024
@dgault
Copy link
Member

dgault commented Jun 25, 2024

Hi @WalkerKnapp, I was wondering if there was any update on potentially getting a public sample file for this PR? If that is not possible then we can arrange to have it shared via Globus.

@dgault dgault added this to the 8.0.0 milestone Jul 8, 2024
@sbesson sbesson added the include label Sep 2, 2024
@sbesson
Copy link
Member

sbesson commented Sep 2, 2024

@WalkerKnapp we have reincluded this PR into the nightly CI builds to assess its impact on the curated QA repository. Assuming builds are passing and in order for us to move forward with the functional review, is there any update from your side on the ability to upload a representative sample to the Zenodo Bio-Formats community?

@pwalczysko
Copy link
Member

imho the inclusion of this PR caused the BF-test-repo to fail in andor-sif folder today

https://merge-ci.openmicroscopy.org/jenkins/job/BIOFORMATS-test-folder/27150/console

@sbesson would you be happy to exclude again please ? This would be of some advantage as we are testing speeds of the tests in prep for the migration to ESS.

@pwalczysko pwalczysko removed the include label Sep 3, 2024
@pwalczysko
Copy link
Member

(I actively went on and removed the Include badge - please re-include if I somehow got this horribly wrong, ta @sbesson )

@pwalczysko
Copy link
Member

After consulation with @sbesson , returning the include label, thank you

@melissalinkert melissalinkert modified the milestones: 8.0.0, 9.0.0, 8.1.0 Sep 30, 2024
@melissalinkert
Copy link
Member

@WalkerKnapp, are you able to provide a file that demonstrates the need for this fix? We would really like to include this change, but we're generally uncomfortable with merging pull requests for which we don't have a test case. If we don't hear anything further, we're likely to close this in the new year (we can always re-open if data appears).

If anyone else who uses Andor SIF has a file that demonstrates this change, and is willing to share it via Zenodo, that would also be enough to move this pull request forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants