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

Add some cautionary statements about adjusting tile sizes #183

Merged
merged 1 commit into from
Jan 26, 2023

Conversation

melissalinkert
Copy link
Member

First try at expanding the tile size documentation, as discussed earlier today. This will also warn if --tile_width or --tile_height is changed from the default at all.

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

(java8) sbesson@Sebastiens-MacBook-Pro-2 Downloads % ./bioformats2raw-0.7.0-SNAPSHOT/bin/bioformats2raw test.fake test.zarr -h 256 -w 512
2023-01-23 09:31:42,862 [main] WARN  c.g.bioformats2raw.Converter - Non-default tile size: 512 x 256. This may cause performance issues in some applications.
(java8) sbesson@Sebastiens-MacBook-Pro-2 Downloads % cat test.zarr/.zattrs 
{
  "bioformats2raw.layout" : 3
}%                         
(java8) sbesson@Sebastiens-MacBook-Pro-2 Downloads % cat test.zarr/0/0/.zarray 
{
  "chunks" : [ 1, 1, 1, 256, 512 ],
  "compressor" : {
    "clevel" : 5,
    "blocksize" : 0,
    "shuffle" : 1,
    "cname" : "lz4",
    "id" : "blosc"
  },
  "dtype" : "|u1",
  "fill_value" : 0,
  "filters" : null,
  "order" : "C",
  "shape" : [ 1, 1, 1, 512, 512 ],
  "dimension_separator" : "/",
  "zarr_format" : 2
}%      

Testing various combinations of input arguments

(java8) sbesson@Sebastiens-MacBook-Pro-2 Downloads % rm -r test.zarr 
(java8) sbesson@Sebastiens-MacBook-Pro-2 Downloads % ./bioformats2raw-0.7.0-SNAPSHOT/bin/bioformats2raw test.fake test.zarr -h 256       
2023-01-23 09:33:48,554 [main] WARN  c.g.bioformats2raw.Converter - Non-default tile size: 1024 x 256. This may cause performance issues in some applications.
(java8) sbesson@Sebastiens-MacBook-Pro-2 Downloads % rm -r test.zarr                                                               
(java8) sbesson@Sebastiens-MacBook-Pro-2 Downloads % ./bioformats2raw-0.7.0-SNAPSHOT/bin/bioformats2raw test.fake test.zarr -w 256 
2023-01-23 09:33:56,066 [main] WARN  c.g.bioformats2raw.Converter - Non-default tile size: 256 x 1024. This may cause performance issues in some applications.
(java8) sbesson@Sebastiens-MacBook-Pro-2 Downloads % rm -r test.zarr                                                               
(java8) sbesson@Sebastiens-MacBook-Pro-2 Downloads % ./bioformats2raw-0.7.0-SNAPSHOT/bin/bioformats2raw test.fake test.zarr -w 1024
(java8) sbesson@Sebastiens-MacBook-Pro-2 Downloads % rm -r test.zarr 

Code-wise all looks good to me

Copy link
Member

@erindiel erindiel left a comment

Choose a reason for hiding this comment

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

The added descriptions for --tile_width and --tile_height options in --help output are very clear and improve the understanding of related chunk size and read size:

  -h, --tile_height=<tileHeight>
                             Maximum tile height (default: 1024). This is both
                               the chunk size (in Y) when writing Zarr and the
                               tile size used for reading from the original
                               data. Changing the tile size may have
                               performance implications.
  -w, --tile_width=<tileWidth>
                             Maximum tile width (default: 1024). This is both
                               the chunk size (in X) when writing Zarr and the
                               tile size used for reading from the original
                               data. Changing the tile size may have
                               performance implications.

README advice is also clear.

@chris-allan chris-allan merged commit 1f1b17a into glencoesoftware:master Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants