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

Issues 4150 + 4110: Consider ZSTD with pyramid levels and fix integer arithmetic scale problem #4138

Merged
merged 3 commits into from
Feb 8, 2024

Conversation

swg08
Copy link
Contributor

@swg08 swg08 commented Jan 7, 2024

The PR aims to fix issues 4110 and 4150 that are concerned with CZIs and pyramid levels.

Issue 4110: Consider ZSTD_0 and ZSTD_1 compressed datasets to also be considered for pyramid level evaluation.
Issue 4150: Fix an integer arithmetic issue by using floating point arithmetic. -> divide with floating point precision and make sure to round to up or down to the next integer value - not truncation rounding.

… considered for pyramid level evaluation.

Issue 4102: Fix an integer arithmetic issue by using floating point arithmetic. -> divide with floating point precision and make sure to round to up or down to the next integer value - not truncation rounding.
@swg08
Copy link
Contributor Author

swg08 commented Jan 7, 2024

Hi, I wasn't able to run the bioformats tests locally.
I tested the proposed fixes on two files we received from a customer. I still need to check whether we can share the files.
Issues 4102 and 4110 also have datasets associated with them - but I wasn't able to test those yet. (Will try to check with Sebi directly.)
Please let me know what I still need to do to move this forward.
Thanks!

@dgault
Copy link
Member

dgault commented Jan 8, 2024

Hi @swg08, thank you for opening the PR. I will include this PR in our CI process to see the extent of the impact it has on the tests. I can also test it against the sample files from the 2 open issues. If you also get permission to share further sample files that would be great.

Also, since your previous contributions we have introduced 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

@dgault dgault added the include label Jan 8, 2024
@swg08
Copy link
Contributor Author

swg08 commented Jan 14, 2024

Hi @dgault,
can you point me to some documentation/page where I can find information about how to build the bioformats.jar and update the fiji jar package? I'm currently testing my changes with some clumsy workflow using bio-formats-tools and the ImageConverter -> converting a CZI to OME.TIFF and then open it in a current FIJI version.
My assumption - which I hope is valid - is that by fixing bugs in the reader we can create a "correct OME.TIFF" which should open up in FIJI without showing any problems - in contrast to opening the CZI which will go wrong in Fiji without the fixes.

If you have any information at hand that describes how improve this rather clumsy workflow that would be great. ;) I was not successful to find the information on the bioformats developer documentation.

(I have some trouble building the test-suite. It complains about certain packages and that the java folder is wrong. I just closed the project and could build all other parts.)

I want to do some more testing, but was concerned that I don't exactly test opening a CZI, but first converting the CZI into an OME.TIFF file which might be a bit different.

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/pb-when-i-open-czi-stitching-image/87585/21

@dgault
Copy link
Member

dgault commented Jan 15, 2024

@swg08, testing using a conversion workflow to OME-TIFF should indeed be testing the CZI reader, though as you say this is a bit cumbersome. For building the Bio-Formats jars I would recommend using Maven, from the top level bioformats folder you should be able to build all the required jars using 'mvn clean package'. If you are seeing any errors or failures just let me know and we can try and help you resolve them.

Depending on your preference for ImageJ or FIJI then you can update the jars using these instructions.

For FIJI you only need to update the single component that contains the CZI Reader, which is the formats-gpl jar. To do this you can replace the existing jar in your FIJI installation at the location below. There are more detailed instructions on doing a full manual install here but that shouldn't be necessary, only the single jar needs replaced:

FIJI > jars > bioformats > replace formats-gpl jar

For ImageJ you can simply use the bioformats_package.jar that conatins the complete set of all of components and dependencies. This is the. Place it as below (or see here for more details)

ImageJ > plugins > bioformats_package.jar

@melissalinkert melissalinkert added this to the 7.2.0 milestone Jan 15, 2024
@swg08
Copy link
Contributor Author

swg08 commented Jan 17, 2024

I will try that. Thanks @dgault!

Copy link
Member

@dgault dgault left a comment

Choose a reason for hiding this comment

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

All builds and repository tests have remained green with this PR included. I retested the sample files from the 2 GitHub Issues. The issue with the ZSTD pyramid levels looks to be resolved, but I am still getting the same results as previously for #4102

For Issue #4110
With and without the PR and testing TR_M=29_uncompressd.czi shows 5 resolution levels

	Resolutions = 5
		sizeX[0] = 10780
		sizeX[1] = 5390
		sizeX[2] = 2695
		sizeX[3] = 1347
		sizeX[4] = 673

Without the PR testing TR_M=29_ZSTD.czi results in just a single image with 1 resolution level:

Series count = 1
Series #0 :
	Image count = 1
	RGB = false (1) 
	Interleaved = false
	Indexed = true (false color, 16-bit LUT: 3 x 65536)
	Width = 10780
	Height = 9063

With this PR included and retesting TR_M=29_ZSTD.czi it now correctly shows the 5 resolution levels:

Series #0 :
	Resolutions = 5
		sizeX[0] = 10780
		sizeX[1] = 5390
		sizeX[2] = 2695
		sizeX[3] = 1347
		sizeX[4] = 673

Then testing #4102 using the sample file https://zenodo.org/record/8423633

With this PR included and the autostitch option enabled, I am still seeing the same results as reported in the Issue, seven series (5 images plus label and macro). The 5 images have differing numbers of resolutions and resolution sizes:

Series count = 7
Series #0 :
	Resolutions = 7
		sizeX[0] = 33309
		sizeX[1] = 16654
		sizeX[2] = 8327
		sizeX[3] = 4163
		sizeX[4] = 2081
		sizeX[5] = 1040
		sizeX[6] = 520
	Image count = 1
	RGB = true (3) 
	Interleaved = true
	Indexed = false (false color)
	Width = 33309
	Height = 29285
	SizeZ = 1
	SizeT = 1
	SizeC = 3 (effectively 1)
	Tile size = 1024 x 1024
	Thumbnail size = 128 x 112
	Endianness = intel (little)
	Dimension order = XYCZT (certain)
	Pixel type = uint8
	Valid bits per pixel = 8
	Metadata complete = false
	Thumbnail series = false
	-----
	Plane #0 <=> Z 0, C 0, T 0

Series #1 :
	Resolutions = 7
		sizeX[0] = 33357
		sizeX[1] = 16678
		sizeX[2] = 8339
		sizeX[3] = 4169
		sizeX[4] = 2084
		sizeX[5] = 1042
		sizeX[6] = 521
	Image count = 1
	RGB = true (3) 
	Interleaved = true
	Indexed = false (false color)
	Width = 33357
	Height = 29295
	SizeZ = 1
	SizeT = 1
	SizeC = 3 (effectively 1)
	Tile size = 1024 x 1024
	Thumbnail size = 128 x 112
	Endianness = intel (little)
	Dimension order = XYCZT (certain)
	Pixel type = uint8
	Valid bits per pixel = 8
	Metadata complete = false
	Thumbnail series = false
	-----
	Plane #0 <=> Z 0, C 0, T 0

Series #2 :
	Resolutions = 7
		sizeX[0] = 33331
		sizeX[1] = 16665
		sizeX[2] = 8332
		sizeX[3] = 4166
		sizeX[4] = 2083
		sizeX[5] = 1041
		sizeX[6] = 520
	Image count = 1
	RGB = true (3) 
	Interleaved = true
	Indexed = false (false color)
	Width = 33331
	Height = 20698
	SizeZ = 1
	SizeT = 1
	SizeC = 3 (effectively 1)
	Tile size = 1024 x 1024
	Thumbnail size = 128 x 79
	Endianness = intel (little)
	Dimension order = XYCZT (certain)
	Pixel type = uint8
	Valid bits per pixel = 8
	Metadata complete = false
	Thumbnail series = false
	-----
	Plane #0 <=> Z 0, C 0, T 0

Series #3 :
	Resolutions = 6
		sizeX[0] = 26093
		sizeX[1] = 13046
		sizeX[2] = 6523
		sizeX[3] = 3261
		sizeX[4] = 1630
		sizeX[5] = 815
	Image count = 1
	RGB = true (3) 
	Interleaved = true
	Indexed = false (false color)
	Width = 26093
	Height = 5576
	SizeZ = 1
	SizeT = 1
	SizeC = 3 (effectively 1)
	Tile size = 1024 x 1024
	Thumbnail size = 128 x 27
	Endianness = intel (little)
	Dimension order = XYCZT (certain)
	Pixel type = uint8
	Valid bits per pixel = 8
	Metadata complete = false
	Thumbnail series = false
	-----
	Plane #0 <=> Z 0, C 0, T 0

Series #4 :
	Resolutions = 7
		sizeX[0] = 34784
		sizeX[1] = 17392
		sizeX[2] = 8696
		sizeX[3] = 4348
		sizeX[4] = 2174
		sizeX[5] = 1087
		sizeX[6] = 543
	Image count = 1
	RGB = true (3) 
	Interleaved = true
	Indexed = false (false color)
	Width = 34784
	Height = 24965
	SizeZ = 1
	SizeT = 1
	SizeC = 3 (effectively 1)
	Tile size = 1024 x 1024
	Thumbnail size = 128 x 91
	Endianness = intel (little)
	Dimension order = XYCZT (certain)
	Pixel type = uint8
	Valid bits per pixel = 8
	Metadata complete = false
	Thumbnail series = false
	-----
	Plane #0 <=> Z 0, C 0, T 0

Series #5 :
	Image count = 1
	RGB = true (3) 
	Interleaved = true
	Indexed = false (false color)
	Width = 698
	Height = 649
	SizeZ = 1
	SizeT = 1
	SizeC = 3 (effectively 1)
	Tile size = 12 x 9
	Thumbnail size = 128 x 119
	Endianness = intel (little)
	Dimension order = XYCZT (certain)
	Pixel type = uint16
	Valid bits per pixel = 12
	Metadata complete = false
	Thumbnail series = true
	-----
	Plane #0 <=> Z 0, C 0, T 0

Series #6 :
	Image count = 1
	RGB = true (3) 
	Interleaved = true
	Indexed = false (false color)
	Width = 1458
	Height = 777
	SizeZ = 1
	SizeT = 1
	SizeC = 3 (effectively 1)
	Tile size = 1458 x 119
	Thumbnail size = 128 x 68
	Endianness = intel (little)
	Dimension order = XYCZT (certain)
	Pixel type = uint16
	Valid bits per pixel = 12
	Metadata complete = false
	Thumbnail series = true
	-----
	Plane #0 <=> Z 0, C 0, T 0

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/pb-when-i-open-czi-stitching-image/87585/26

@swg08
Copy link
Contributor Author

swg08 commented Jan 24, 2024

@dgault
Thank you for the log!
I will have a deeper look until the end of the week.
I was travelling and couldn't look into the topic again yet.
-> I have some good news. We received the allowance of the customer to use the image we received from him and contribute it to OME/BioFormats. (actually 3 datasets, ~3.5 GB in total)
We think the issue is very much related to #4102.
Are you interested to ingest this dataset? Where can I upload it to?

@dgault
Copy link
Member

dgault commented Jan 25, 2024

Thanks @swg08, if you are able to share the dataset publicly then uploading https://zenodo.org/ would be great. If not then we can try and arrange an ftp transfer.

@swg08
Copy link
Contributor Author

swg08 commented Jan 28, 2024

@dgault I'm on it. One file upload keeps on stalling.

Regarding your observations: I think I understand what is going on now.

-> I was focussing on this comment https://forum.image.sc/t/pb-when-i-open-czi-stitching-image/87585/3?u=swg08 to relate the changes regarding the rounding to 4102.

-> When checking the issue description for 4102 again - and also the according discussion - I'm actually not sure at the moment if that is a feature request - or what the actual problem is. ;)

If it helps, we can create another issue for the rounding topic and change the description and links in the PR.


When checking the file TEST 2023_10_10__1046.czi I didn't observe the rounding problem. So, you are absolutely right with your findings. -> The observation regarding number of scales/pyramid levels is also fine. The one image is just a bit smaller and therefore one less pyramid level has been generated.

Two potential topics for 4102 remain:

  • being able to see all scenes in one view -> that's not something that autostitch currently addresses as far as I understand autostitch
  • scrambled display in one/some of the resolution levels?

@swg08
Copy link
Contributor Author

swg08 commented Jan 28, 2024

@dgault Here we go: https://zenodo.org/records/10577186

There are 3 files. One original scan and two images that have been derived from it via processing.

2023_11_30__RecognizedCode-27.czi -> original image, JPGXR compressed, containing pyramid levels
2023_11_30__RecognizedCode-27-Background subtraction-08.czi -> subtracted background, no compression, containing pyramid levels
2023_11_30__RecognizedCode-27-Deconvolution (defaults)-11.czi -> deconvolved image, no compression, containing pyramid levels

@dgault
Copy link
Member

dgault commented Jan 30, 2024

Thanks @swg08 for linking the sample files, that is a gret help! I tried testing the new sample files alongside one of the previously reported files. For all of the files, even with the autostitch option enabled it does not seem to return a stitched image. Are you seeing the same thing with this PR and autostitch enabled?

I went back and retested the 20231016_dia1mthspax7DIApax7 1mthsA.czi sample file from https://forum.image.sc/t/pb-when-i-open-czi-stitching-image/87585

Without this PR and autostitch disabled results in 24 series, all sized 2236 x 2236 x 4
Without this PR and autostitch enabled resulted in 3 series (11516 x 7776 x 4, 3838 x 2592 x 4, 1279 x 864 x 4)
With this PR and autostitch disabled results in 24 series, all sized 2236 x 2236 x 4
With this PR and autostitch enabled results in 24 series, all sized 2236 x 2236 x 4

Testing the new sample files:

For 2023_11_30__RecognizedCode-27-Background subtraction-08.czi:
Without this PR and autostitch disabled results in 266 series, all 1054 x 2048 plus a macro and label
Without this PR and autostitch enabled results in 7 series, 5 different resolutions (1504 x 2048 ... 94 x 128) plus a macro and label
With this PR and autostitch disabled results in 266 series, all 1054 x 2048 plus a macro and label
With this PR and autostitch enabled results in 266 series, all 1054 x 2048 plus a macro and label

For 2023_11_30__RecognizedCode-27-Deconvolution (defaults)-11.czi:
Without this PR and autostitch disabled results in 266 series, all 1054 x 2048 plus a macro and label
Without this PR and autostitch enabled results in 7 series, 5 different resolutions (1504 x 2048 ... 94 x 128) plus a macro and label
With this PR and autostitch disabled results in 266 series, all 1054 x 2048 plus a macro and label
With this PR and autostitch enabled results in 266 series, all 1054 x 2048 plus a macro and label

For 2023_11_30__RecognizedCode-27.czi:
Without this PR and autostitch disabled results in 266 series, all 1054 x 2048 plus a macro and label
Without this PR and autostitch enabled results in 8 series, 6 different resolutions (32751 x 20684 ... 1023 x 646) plus a macro and label
With this PR and autostitch disabled results in 266 series, all 1054 x 2048 plus a macro and label
With this PR and autostitch enabled results in 266 series, all 1054 x 2048 plus a macro and label

@swg08
Copy link
Contributor Author

swg08 commented Jan 30, 2024

Great, it seems I broke AutoStitch... let me check this once more.
Thank you for the testing @dgault !!

@swg08
Copy link
Contributor Author

swg08 commented Feb 2, 2024

@dgault
I'm actually not seeing the series when AutoStitch is turned on.
I'm doing the following for 2023_11_30__RecognizedCode-27-Deconvolution (defaults)-11.czi:
Open ome.tiff file that has been converted from CZI file with BioFormats in Fiji with those settings:

image

AutoStitch is set to true:

image

Then I can open 8 series:
image

Thumbnails are not being loaded and there is an exception in the output console:
image

Do I need to restart Fiji when turning the AutoStitch option off? (I get the same results -> just 8 series... not 266 :O)

@NicoKiaru
Copy link
Contributor

@swg08 I don't think you need to click Stitch tiles, but I may be wrong (I have to admit I never checked this box. and don't know what it is doing ... )

For the autostitch option to go on or off, there's no need to restart Fiji

@dgault
Copy link
Member

dgault commented Feb 2, 2024

Yeah you shouldn't need to restart FIJI for the autostitch option. The exception is not one I have seen before though, it doesn't appear to be coming directly from the reader.

I retested again today and realised that my previous test results may have been due to having memo files created beside the sample files which prevented the autostitching. This time the results matched what I would expect with the correct autostitching, all the images could be opened and displayed correctly also. Overall these results look to be as I would expect.

For 20231016_dia1mthspax7DIApax7 1mthsA.czi
With this PR and autostitch disabled results in 24 series, all sized 2236 x 2236 x 4
With this PR and autostitch enabled results in 5 series, (11516 x 7776 x 4, 5758 x 3838 x 4, 2879 x 1944 x 4, 1439 x 932 x 4, 719 x 486 x 4)

Screenshot 2024-02-02 at 17 38 11

For 2023_11_30__RecognizedCode-27-Background subtraction-08.czi:
With this PR and autostitch disabled results in 266 series, all 1054 x 2048 plus a macro and label
With this PR and autostitch enabled results in 8 series, 6 different resolutions (32751 x 20684 ... 1023 x 646) plus a macro and label

Screenshot 2024-02-02 at 17 33 32

For 2023_11_30__RecognizedCode-27-Deconvolution (defaults)-11.czi:
With this PR and autostitch disabled results in 266 series, all 1054 x 2048 plus a macro and label
With this PR and autostitch enabled results in 8 series, 6 different resolutions (32751 x 20684 ... 1023 x 646) plus a macro and label

Screenshot 2024-02-02 at 17 34 52

For 2023_11_30__RecognizedCode-27.czi:
With this PR and autostitch disabled results in 266 series, all 1054 x 2048 plus a macro and label
With this PR and autostitch enabled results in 8 series, 6 different resolutions (32751 x 20684 ... 1023 x 646) plus a macro and label

Screenshot 2024-02-02 at 17 35 49

@swg08
Copy link
Contributor Author

swg08 commented Feb 2, 2024

@dgault does this now mean that the PR can be completed? (Who will do this? What do I still need to take care of?)

Addtion 2024-02-04: I have a small - unrelated - change that I would like to checkin. Not necessarily as part of this PR, but if it is OK and helps keeping the number of PRs low, I would go ahead and do it. -> It's basically just changing a warning message to a debug message.

@dgault
Copy link
Member

dgault commented Feb 5, 2024

@swg08, yes the PR is looking in a good state. I am going to complete one last bit of testing and then I will get this merged for release this week. If you have a small change for changing a warning message to debug then feel free to include it with this PR, but if needs be we can handle it in a quick follow up PR also.

Change log level of ZSTD1 without hi/lo flag from warning to debug. 
Rationale: The image data is still usable and valid in that case. Changing to debug information.
@swg08
Copy link
Contributor Author

swg08 commented Feb 5, 2024

I merged the latest develop into my branch and did the one logging change. I hope it's okay that way.
Please let me know if there is anything I still need to do. Thank you @dgault !

@dgault dgault changed the title Issues 4102 + 4110: Consider ZSTD with pyramid levels and fix integer arithmetic scale problem Issues 4150 + 4110: Consider ZSTD with pyramid levels and fix integer arithmetic scale problem Feb 7, 2024
@dgault dgault mentioned this pull request Feb 7, 2024
Copy link
Member

@dgault dgault left a comment

Choose a reason for hiding this comment

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

I have split the original Issue #4102 into 2 posts as the 2 reports appear to be seperate. There is a new Issue #4150 which will be resolved by this PR

Otherwise everything included in this PR looks good from my perspective

@dgault dgault merged commit fa80cbd into ome:develop Feb 8, 2024
17 checks passed
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