V1.0fixes #6

MLackner · 2019-02-18T16:02:06Z

I had to change quite a lot for the part where the reader was just skipping lines in the header of the file. I could only test with the files I have at hand. I included these in the test folder. I'm not sure if that's the right way to do it though.

…nto v1.0fixes

timholy · 2019-03-08T13:03:19Z

For some reason src/AndorSIF.jl is listed as a binary file. That makes it difficult to review this PR (click the "Files changed" tab above to see what I mean). Can you explore changing those settings?

MLackner · 2019-05-09T10:51:23Z

should be fixed now

timholy

Thanks for doing this. Just small stuff and then I think this can be merged.

timholy · 2019-05-11T12:52:32Z

src/AndorSIF.jl


-    pixels = read(io, Float32, width, height, frames)
+    # two additional lines?


This is the only part that seems concerning, assuming that it nominally worked before. (I've never tested this myself.) Since it's "weird," is there way to do a sanity check and make sure you're not accidentally consuming offset (below)?

Including either one or both lines yields non-integer values for the test files I have at hand. Also, the data looks correct and if I remember correctly, I checked it against tif files generated alongside the sif files.

timholy · 2019-05-11T12:53:31Z

src/AndorSIF.jl

    prop = Compat.@compat Dict(
        "colorspace" => "Gray",
-        "spatialorder" => ["y", "x"],
+        "spatialorder" => ["x", "y"],


This is pretty outdated now. I'd use permuteddimsview from ImageCore.jl. Likewise, returning Gray{Float32} takes care of the colorspace field. Finally, [1, 1] is the default pixelspacing, better not to include it since folks may want to provide the correct answer (e.g., in microns) using Unitful.jl and AxisArrays.jl. So the only need for the ImageMeta is the ixon header.

timholy · 2019-05-11T12:54:25Z

src/AndorSIF.jl

        "ixon" => ixon,
        "suppress" => Set{Any}(("ixon",)),
        "pixelspacing" => [1, 1]
    )
    if frames > 1
        prop["timedim"] = 3
    end
-    Image(float64(pixels), prop)
+    ImageMeta(pixelmatrix .|> Float64, prop)


I don't see that it's important to return Float64, Gray{Float32} seems entirely appropriate.

timholy · 2019-09-05T15:41:47Z

Bump. Given #7, best to get this merged a new version tagged. If there are some things you're uncertain of, just give it your best shot and I'll merge.

timholy · 2020-03-27T14:12:54Z

Thanks for continuing to take a crack at this. In .travis.yml, change release to 1. See also my review comments; the colorspace, spatialorder, and pixelspacing all dates from Images prior to version 0.6 (we're now on 0.22). See https://discourse.julialang.org/t/big-changes-in-images-jl-v0-6-0/1786 for more info. Unfortunately all the deprecation warnings are long gone (we kept them for about a year, and even that was hard to maintain while still advancing the package), but you could install Julia 0.6 and then Images 0.6 if you want to get them back to help you perform the upgrade.

MLackner · 2020-08-07T11:45:08Z

src/AndorSIF.jl

+    numbytes = 4 * frames * width * height # number of bytes to read
+    pixelbytes = read(io, numbytes)
+    pixelarray = reinterpret(Gray{Float32}, pixelbytes)
+    pixelmatrix = reshape(pixelarray, (height, width)) |> collect


I'm not sure if it is a good idea to collect the reshaped array. But returning the Reshaped type looked a bit messy.

You could also do this:

pixelmatrix = Matrix{Gray{Float32}}(undef, height, width) read!(io, pixelmatrix)

and then it will immediately be in the shape and element type you want.

Nice trick! That made me realize that there currently was no support for multiple frames. Returning a 3D array would be a solution. I don't know if that's the best, though.

If it's continuous on disk, that would be best. Even better would be to use Mmap.mmap: https://docs.julialang.org/en/latest/stdlib/Mmap/, since it would allow you to "load" a terabyte-sized file without saturating your memory. You can see examples in https://github.com/JuliaIO/NRRD.jl/blob/9202ea1d01c69413104e2e7328f2cbaca06f5ee2/src/NRRD.jl#L242-L243.

timholy

This looks largely ready to go, thanks!

timholy · 2020-08-07T15:35:03Z

src/AndorSIF.jl

+    numbytes = 4 * frames * width * height # number of bytes to read
+    pixelbytes = read(io, numbytes)
+    pixelarray = reinterpret(Gray{Float32}, pixelbytes)
+    pixelmatrix = reshape(pixelarray, (height, width)) |> collect


You could also do this:

pixelmatrix = Matrix{Gray{Float32}}(undef, height, width) read!(io, pixelmatrix)

and then it will immediately be in the shape and element type you want.

timholy · 2020-08-07T16:06:07Z

I think we could merge this as-is, unless you want to do the Mmap thing now. It could be a separate PR if you prefer. Just let me know.

MLackner · 2020-08-10T07:36:21Z

I think we could merge this as-is, unless you want to do the Mmap thing now. It could be a separate PR if you prefer. Just let me know.

With Mmap thing you mean the pixelmatrix = Matrix{Gray{Float32}}(undef, height, width)? That should be in now. I would say ready to merge.

timholy · 2020-08-10T11:12:53Z

Well, the mmap could replace that. Here's the issue: Matrix{T}(undef, height, width) requires that you have an amount of RAM in your computer to store height*width objects of size sizeof(T). What if you don't have enough RAM? Well, in principle it should be solvable because the raw data are stored on disk: if you're playing a movie, for example, you only need to have the frame you're viewing at the current time, the rest of the data could stay on disk and you just load what you need "on demand."

That's what memory-mapping does: it "fakes" a giant array, without requiring that you be able to load the whole thing at once. Instead, data are loaded on-demand, but you get the convenience of not having to worry about that and getting to treat the array as if it's just a regular array.

timholy · 2020-08-10T11:18:10Z

Hmm, got this error: ef3d444#commitcomment-41335099

From here you can see that even 0.0.3 was released. I've changed this to v0.1.0 and submitted a registration, see JuliaRegistries/General#19264. Because this is now counted as a new package, it will take 3 days before that is merged.

MLackner added 10 commits February 18, 2019 12:47

Added Project.toml + Test

c0e9527

Fixes for 1.0 + further test

d25a86c

make temperature field a float

2a11726

make temperature float JuliaIO#2

5620f59

make temperature float JuliaIO#3

7b6b0bd

give temporary version

e57b55d

swapped width and height

8fb4efb

undo swapped width and height

26cc506

Merge branch 'v1.0fixes' of https://github.com/MLackner/AndorSIF.jl i…

0435609

…nto v1.0fixes

swap width and height

d49b443

fix showing up as binary

5b4bc03

timholy reviewed May 11, 2019

View reviewed changes

fix dependencies

86499e8

MLackner added 2 commits August 7, 2020 13:35

fixed ImageMeta; return eltype Gray{Float32}

95872e7

fixed julia version in CI

5e0db40

MLackner commented Aug 7, 2020

View reviewed changes

removed compatibility constraints

5a9dcb9

timholy reviewed Aug 7, 2020

View reviewed changes

fixed multiple frames

40a52c5

timholy merged commit ef3d444 into JuliaIO:master Aug 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V1.0fixes #6

V1.0fixes #6

MLackner commented Feb 18, 2019

timholy commented Mar 8, 2019

MLackner commented May 9, 2019

timholy left a comment

timholy May 11, 2019

MLackner Aug 7, 2020

timholy May 11, 2019

timholy May 11, 2019 •

edited

Loading

timholy commented Sep 5, 2019

timholy commented Mar 27, 2020

MLackner Aug 7, 2020

timholy Aug 7, 2020

MLackner Aug 7, 2020

timholy Aug 7, 2020

timholy left a comment

timholy Aug 7, 2020

timholy commented Aug 7, 2020

MLackner commented Aug 10, 2020

timholy commented Aug 10, 2020

timholy commented Aug 10, 2020


		pixels = read(io, Float32, width, height, frames)
		# two additional lines?

V1.0fixes #6

V1.0fixes #6

Conversation

MLackner commented Feb 18, 2019

timholy commented Mar 8, 2019

MLackner commented May 9, 2019

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy May 11, 2019 • edited Loading

Choose a reason for hiding this comment

timholy commented Sep 5, 2019

timholy commented Mar 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy commented Aug 7, 2020

MLackner commented Aug 10, 2020

timholy commented Aug 10, 2020

timholy commented Aug 10, 2020

timholy May 11, 2019 •

edited

Loading