-
Notifications
You must be signed in to change notification settings - Fork 241
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
Reworking LMSMetadata package and adding a new lif reader #4000
base: develop
Are you sure you want to change the base?
Reworking LMSMetadata package and adding a new lif reader #4000
Conversation
…t allowed in prolog. adapted error messaging
…h uses LMS metadata classes (pending discussion). temporary changes to LIFReader.isThisType() and readers.txt to use LIFReader2 instead of LIFReader for testing
…ectors and filters translation
…sequential list to channel detector settings
… blocks to detector settings and channels
…rchitecture (in progress)
…al Z size translation
…stinguishes between SPX and STELLARIS, fixing channelPrios for LIF/LOF
…tializer, MetadataTempBuffer
…instruments for images without hardware settings, empty timestamps, correct interleaving depending on image format
…plane calculation, adding z drive modes
…al and non-sequential widefield images
This PR was included in the CI last night, unfortunately a number of files threw exceptions at the setID stage so the full test suite didn't actually get to run. I have only had a quick look at the failures but a lot of them look to be NullPointerExceptions. Unfortunately the files that are failing are not ones that are public. The full console output from the test run is at: |
hi @dgault, okay thanks for sharing, I'll try to fix all the issues where exceptions are thrown. |
hi @dgault , I tried to address all the exceptions that happened in the tests, trying to increase the robustness against unexpected input. |
@XLEFReaderForBioformats I'm curious to know whether your request will be approved because I've had exactly the same issue in here: This point has also been raised and answered in the image.sc forum: |
@NicoKiaru thanks for sharing, good to know... @dgault I have a suggestion... I assume that the problems of the new reader with your test data arise from unexpected XML layout and not so much from the image binary data itself. Would it be an option for you to share only the image XML with me privately? If you have only LIF files available, you could copy the XML using a hex editor, or alternatively you could e.g. use a LASX Offline to open LIF files and save them as e.g. XLEF/TIF files, and then it would suffice for me if you sent me the XLIF files, which can be found in metadata folders. Having the image XML, I can create some "fake" image datasets with it for local testing. Would this be an option? |
…properties"), wrong image offsets are used mapping of memory blocks to images found in XML via memory block ids, removing offsets of "invalid" memory blocks. frame properties and other children of image nodes are not considered valid images
hi @dgault @melissalinkert, |
@XLEFReaderForBioformats, Im not sure why those jobs failed in that way but I re-triggered them again and they should now all be green |
@dgault I'd like to ask if there are any updates regarding the test data? It would be helpful for me to know at least if you are planning to share any data with me or not, right now it is a bit unclear to me how to proceed with this PR. |
Hi @XLEFReaderForBioformats, the next step will be to reinclude the PR in the nightly CI, testing the new reader rather than the legacy reader. If we can get a full run of the test suite then we will at least be able to identify the number of files which are causing failures and judge the scale of the impact. From there we can decide how best to proceed. I will re-include the PR now so we can have the new reader in the nightly CI for the purposes of the testing. |
Hi @XLEFReaderForBioformats, as a follow up, the PR has been included in our test suite for a number of days and we are seeing failures for a number of files, though most are for the same exception. You can see the LIF failures here:
The failures seem to be caused by the following line when retrieving the offsets: The following sample files which failed are available for testing from here. These should be good samples to test for the IndexOutOfBoundsException
|
Hi @dgault, sorry for the long delay on my side! After a longer phase of release support that I had to prioritize, I can now hopefully return to finishing this PR, and I'm really looking forward to it! I fixed two bugs that led to the exceptions in the test log that you kindly provided. Since it's been a few months since the last update from my side, I am unsure as to how we best proceed now. Would you simply include the PR in your test suite again? |
Hi @melissalinkert , I'd like to kindly ask again how we can proceed with this PR, as I fixed all issues that were found in your automated tests. |
Main changes:
In general, metadata translation will be an ongoing topic here, and there might also be images from systems that are not yet fully regarded and where metadata translation needs to be improved. I'll try to fix all major bugs that might be noticed in the context of this PR, but concerning minor bugs and "missing metadata" , I guess I have to discuss with my colleagues how much of that I will be able to fix within this PR.
@dgault as we have discussed before, since you suggested that we could use a DelegateReader here to provide both the existing LIFReader and the new LMSLIFReader to users, I think that's a reasonable approach, so this PR at least won't create a deterioration of existing .lif files reading.
I removed my temporal changes to readers.txt and LIFReader.isThisType(...) for testing so now the LIFReader is the standard reader for .lif files again. Could you let me know how using a delegate reader would look like so both lif readers can be used?
Please let me know if you need any additional information or explanations from my side.