HsupoLeng commented 2 years ago

Hi there,

I recently started using OMETIFF for dealing with my microscope recordings in Julia. The package worked great for most of my data, but failed in the case where I collected multiple cycles of time series. Each cycle share the same imaging parameters, and were collected as automatic repetitions over the same FOV. As a neuroscientist, I perform this type of acquisition as I change the experiment condition for each cycle, so I can ask the effect of experiment condition on the neurons in the FOV. This should be a fairly general user case.

Back to the package, I looked into the code and seemed to have found an one-line bug with potentially broad impact.

Error

When there are multiple image nodes in the same ome.tiff, calling load() results in BoundsError. See error message below.

Root cause

When there are multiple image nodes (containers) in the dataset, the function ifdindex! is called for the corresponding number of times. ifd_indices and two other outputs are shared across ifdindex! calls, but in each call, the indices ifd and prev_ifd always restart from 1 and 0. As a result, the outputs have the size equal to only one container; information from later image nodes overwrites earlier ones. At the same time, the last index in ifd_indices (pos_idx) increments to track container/Image ID. When ifd_indices were later used to access an one-container-sized array in function inmemoryarray, BoundsError is triggered.

Fix

Initialize prev_ifd in ifdindex! as the length of obs_filepaths, to reflect the number of existing ifds.

Test

The fixed version passes all existing tests in runtests.jl. I have provided a folder of dummy images that should allow the error to be reproduced here, but have not included them in a test, because of the relatively large number of images.

julia> imgmeta = load(raw"TSeries_camp-000-ometiff_test\TSeries_camp-000_Cycle00001_Ch1_000001.ome.tif")
Error encountered while load File{DataFormat{:OMETIFF}, String}("TSeries_camp-000-ometiff_test\\TSeries_camp-000_Cycle00001_Ch1_000001.ome.tif").

Fatal error:
ERROR: BoundsError: attempt to access 512×512×1×2×11×1 Array{Gray{N0f16},6} with eltype ColorTypes.Gray{FixedPointNumbers.N0f16} at index [1:512, 1:512, 1, 1, 1, 3]
Stacktrace:
  [1] throw_boundserror(A::Array{ColorTypes.Gray{FixedPointNumbers.N0f16}, 6}, I::Tuple{Base.Slice{Base.OneTo{Int64}}, Base.Slice{Base.OneTo{Int64}}, Int64, Int64, Int64, Int64})
    @ Base .\abstractarray.jl:651
  [2] checkbounds
    @ .\abstractarray.jl:616 [inlined]
  [3] view(::Array{ColorTypes.Gray{FixedPointNumbers.N0f16}, 6}, ::Function, ::Function, ::Int64, ::Int64, ::Int64, ::Int64)
    @ Base .\subarray.jl:177
  [4] maybeview(::Array{ColorTypes.Gray{FixedPointNumbers.N0f16}, 6}, ::Function, ::Function, ::Vararg{Any, N} where N)
    @ Base .\views.jl:133
  [5] dotview(::Array{ColorTypes.Gray{FixedPointNumbers.N0f16}, 6}, ::Function, ::Function, ::Vararg{Any, N} where N)
    @ Base.Broadcast .\broadcast.jl:1212
  [6] macro expansion
    @ D:\.julia\dev\OMETIFF\src\loader.jl:136 [inlined]
  [7] macro expansion
    @ D:\.julia\packages\ProgressMeter\Vf8un\src\ProgressMeter.jl:940 [inlined]
  [8] inmemoryarray(ifds::OrderedCollections.OrderedDict{NTuple{4, Int64}, Tuple{TiffImages.TiffFile{UInt32, S} where S<:Stream, TiffImages.IFD{UInt32}}}, dims::NamedTuple{(:Y, :X, :Z, :C, :T, :P), NTuple{6, Int64}}; verbose::Bool)
    @ OMETIFF D:\.julia\dev\OMETIFF\src\loader.jl:134
  [9] load(io::Stream{DataFormat{:OMETIFF}, IOStream, String}; dropunused::Bool, verbose::Bool, inmemory::Bool)
    @ OMETIFF D:\.julia\dev\OMETIFF\src\loader.jl:94
 [10] #31
    @ D:\.julia\dev\OMETIFF\src\loader.jl:3 [inlined]
 [11] open(f::OMETIFF.var"#31#32"{Bool, Bool}, args::File{DataFormat{:OMETIFF}, String}; kwargs::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ Base .\io.jl:330
 [12] open
    @ .\io.jl:328 [inlined]
 [13] #load#30
    @ D:\.julia\dev\OMETIFF\src\loader.jl:2 [inlined]
 [14] load(f::File{DataFormat{:OMETIFF}, String})
    @ OMETIFF D:\.julia\dev\OMETIFF\src\loader.jl:2
 [15] #invokelatest#2
    @ .\essentials.jl:708 [inlined]
 [16] invokelatest
    @ .\essentials.jl:706 [inlined]
 [17] action(::Symbol, ::Vector{Union{Base.PkgId, Module}}, ::Formatted; options::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ FileIO D:\.julia\packages\FileIO\QkYgA\src\loadsave.jl:219
 [18] action
    @ D:\.julia\packages\FileIO\QkYgA\src\loadsave.jl:197 [inlined]
 [19] action(::Symbol, ::Vector{Union{Base.PkgId, Module}}, ::Symbol, ::String; options::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ FileIO D:\.julia\packages\FileIO\QkYgA\src\loadsave.jl:185
 [20] action
    @ D:\.julia\packages\FileIO\QkYgA\src\loadsave.jl:185 [inlined]
 [21] load(::String; options::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
    @ FileIO D:\.julia\packages\FileIO\QkYgA\src\loadsave.jl:113
 [22] load(::String)
    @ FileIO D:\.julia\packages\FileIO\QkYgA\src\loadsave.jl:110
 [23] top-level scope
    @ REPL[10]:1
Stacktrace:
 [1] handle_error(e::BoundsError, q::Base.PkgId, bt::Vector{Union{Ptr{Nothing}, Base.InterpreterIP}})
   @ FileIO D:\.julia\packages\FileIO\QkYgA\src\error_handling.jl:61
 [2] handle_exceptions(exceptions::Vector{Tuple{Any, Union{Base.PkgId, Module}, Vector{T} where T}}, action::String)
   @ FileIO D:\.julia\packages\FileIO\QkYgA\src\error_handling.jl:56
 [3] action(::Symbol, ::Vector{Union{Base.PkgId, Module}}, ::Formatted; options::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ FileIO D:\.julia\packages\FileIO\QkYgA\src\loadsave.jl:228
 [4] action
   @ D:\.julia\packages\FileIO\QkYgA\src\loadsave.jl:197 [inlined]
 [5] action(::Symbol, ::Vector{Union{Base.PkgId, Module}}, ::Symbol, ::String; options::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ FileIO D:\.julia\packages\FileIO\QkYgA\src\loadsave.jl:185
 [6] action
   @ D:\.julia\packages\FileIO\QkYgA\src\loadsave.jl:185 [inlined]
 [7] load(::String; options::Base.Iterators.Pairs{Union{}, Union{}, Tuple{}, NamedTuple{(), Tuple{}}})
   @ FileIO D:\.julia\packages\FileIO\QkYgA\src\loadsave.jl:113
 [8] load(::String)
   @ FileIO D:\.julia\packages\FileIO\QkYgA\src\loadsave.jl:110
 [9] top-level scope
   @ REPL[10]:1

julia>

codecov[bot] commented 2 years ago

Codecov Report

Merging #94 (3aa57d1) into master (0065ef6) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #94   +/-   ##
=======================================
  Coverage   95.07%   95.07%           
=======================================
  Files           6        6           
  Lines         264      264           
=======================================
  Hits          251      251           
  Misses         13       13

Impacted Files	Coverage Δ
src/parsing.jl	`97.50% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0065ef6...3aa57d1. Read the comment docs.