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

bugs #tokazama-imagemeta #21

Open
korbinian90 opened this issue Feb 17, 2019 · 8 comments
Open

bugs #tokazama-imagemeta #21

korbinian90 opened this issue Feb 17, 2019 · 8 comments

Comments

@korbinian90
Copy link
Contributor

First, the new NIfTI integration with JuliaImages looks awesome!
It looks to me like it's close to beeing finished, so here are the things that I saw when trying it:

I wasn't able to open my nifti files yet (not zipped), but I can open your sample files.
I found the following issues:

  1. my NIfTI files have no units attached to them. When I open them following error results from multiplication with nothing
    ERROR: MethodError: no method matching * ::StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}, ::Nothing)
    I think the problem is that in traits.jl Line 23 get(NiftiUnits, hdr.xyzt_units & 0x07, 1) does not default to 1 when nothing is returned, only when the entry is missing.
    After removing the entry (Int16(0), nothing) from the dict that part seemed to work. Or does it make more sense to use the u"" for no units?

  2. read.jl Line 94: "qformcode" => sformcode(hdr),
    There appears to be a typo, I guess it should mean qformcode(hdr)

  3. Another thing I realized while adding the NIfTI package was that the following other packages are not specified in the REQUIRE file and need to be added manually:
    StaticArrays
    MappedArrays
    GeometryTypes
    Rotations

  4. The next Error that occurs for my files is:

ERROR: EOFError: read end of file
[1] unsafe_read(::TranscodingStreams.TranscodingStream{TranscodingStreams.Noop,IOStream}, ::Ptr{UInt8}, ::UInt64) at C:\Users\korbi\.julia\packages\TranscodingStreams\SaPZ8\src\noop.jl:92
[2] macro expansion at .\io.jl:585 [inlined]
[3] read! at .\io.jl:603 [inlined]
[4] #niread#28(::Bool, ::Function, ::TranscodingStreams.TranscodingStream{TranscodingStreams.Noop,IOStream}, ::NiftiSchema{Tuple{448,448,112,3},Int16,Int16}, ::Type{Array}) at C:\Users\korbi\julia\dev\NIfTI\src\read.jl:203
@Tokazama
Copy link
Member

Thanks for taking the time to look at all of this!

  1. This seems reasonable to me.
  2. This is definitely a typo, but I'm thinking about just reading straight to a dict and avoiding this exact function all together as I think it's unlikely enough people will be motivated to maintain a unique set of NIfTI like functions for a NIfTI header struct and ImageMeta. This is a great example, as a lot of software just defaults to qform or sform and ignores the other because it's too hard to figure out what everyone wants to do with it.
  3. Yes, this will be fixed. I've just been lazy.
  4. I think this may be related a work around I was trying to use to make CodecZlib work. It's suppose to be the fastest at gzip decompression right now, but it lacks a seek method for streams and that's really helpful for working around NIfTI's bizarre extension region. I'll probably just drop CodecZlib until someone creates a workable solution.

@tknopp
Copy link
Contributor

tknopp commented Apr 16, 2019

@Tokazama: Any update on this? The design of this looks pretty good and I tested reading which seemed to work fine. Writing however not. I get ERROR: UndefVarError: scl_sclope not defined if I call niwrite.

@tknopp
Copy link
Contributor

tknopp commented Apr 17, 2019

@Tokazama: I am trying to get the tests running but ran into several issues.

  • First it seems that there are several files that are not checked in. Could you add them?
  • Then I needed to comment in fileio.jl but I get LoadError: File{DataFormat{:UNKNOWN}}
  • The load functions seem to never call niread. Isn't that what they are supposed to do?

@Tokazama
Copy link
Member

Sorry for the delayed response. I'll take a look at this over the course of today.

@Tokazama
Copy link
Member

I'm working on fixing the errors mentioned here and writing better unit testing. I'll likely have something by the end of the day for further review. I'm also going to write down some details related to design choice and small changes I'll probably start working on #16. None of these should be user facing so it's likely of little concern to most.

@tknopp
Copy link
Contributor

tknopp commented Apr 19, 2019

Great. Isn't the branch already implementing #16? Or do you refer more to the standardized parameter naming within the ImageMetadata?

@Tokazama
Copy link
Member

It may be worth trying this all out again if your interested. You will need to explicitly add NIfTI to your File IO registry for now ( see the tests for how to do this). Im working on a couple more things to really stress test the new implementation and then it should be good to go.

@tknopp
Copy link
Contributor

tknopp commented Jul 5, 2019

I currently do not have the time to review so go ahead with merging, once you are confident with the design. I would be very interested in learning how to generate a nifti file from scratch. You have one load/ save example but this does not help, if one has just an array at hand.

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

No branches or pull requests

3 participants