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.

Sign up for GitHub

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

Get initial traits in place for NeuroCore #1

Merged
merged 15 commits into from
Dec 27, 2019
Merged

Get initial traits in place for NeuroCore #1

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e6120b3
Initial implementation of BIDS properties.
Tokazama Nov 23, 2019
adc0cd3
Get initial documentation in place before exhaustive testing
Tokazama Dec 12, 2019
eeb07c9
Package loads and ready to get prelim feedback
Tokazama Dec 17, 2019
409383a
Added more defaults and cleaned up dicom tags
Tokazama Dec 17, 2019
d9f0e28
Change to unitful measurements, fixed type, ditch modalities
Tokazama Dec 17, 2019
6175205
Get rid of `maybe_ustrip`
Tokazama Dec 17, 2019
4a67b0f
Update src/coordinates.jl
Tokazama Dec 18, 2019
d614e46
Merge branch 'master' of https://github.com/Tokazama/NeuroCore.jl
Tokazama Dec 18, 2019
98eb90e
Big reorganization of bids entities
Dec 19, 2019
e7c2fb9
reorganize BIDS entities
Dec 20, 2019
7d85781
Add tests and fix enums
Dec 20, 2019
3bda9fa
Using AffineMap instead of 4x4 array now.
Tokazama Dec 21, 2019
3a01694
Test all BIDS Entities
Dec 24, 2019
d33cc03
remove bidsdata and split affinemap/coordinates
Dec 26, 2019
8c01c21
snake case for properties but annotate BIDS mapping
Dec 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Initial implementation of BIDS properties.
  • Loading branch information
@Tokazama
Tokazama committed Nov 23, 2019
commit e6120b3b2f3536e8f42343bd001a8808844e37e6
111 changes: 111 additions & 0 deletions src/NeuroCore.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
module NeuroCore

using ImageCore

export freqdim,
freqdim!,
slicedim,
slicedim!,
phasedim,
phasedim!,
slice_start,
slice_start!,
slice_end,
slice_end!,
slice_duration,
slice_duration!,
echo_time,
echo_time!,
inversion_time,
inversion_time!,
delay_time,
delay_time!,
acquisition_duration,
acquisition_duration!,
delay_after_trigger,
delay_after_trigger!,
start_time,
start_time!,
nvol_user_discarded,
nvol_user_discarded!,
nvol_scanner_discarded,
nvol_scanner_discarded!,
institution_name,
institution_name!,
institution_address,
institution_address!,
institutional_department_name,
institutional_department_name!,
station_name,
station_name!,
manufacturer,
manufacturer!,
manufacturer_model_name,
manufacturer_model_name!,
magnetic_field_strength,
magnetic_field_strength!,
receiver_coil_name,
receiver_coil_name!,
receive_coil_active_elements,
receive_coil_active_elements!,
gradient_set_type,
gradient_set_type!,
matrix_coil_mode,
matrix_coil_mode!,
coil_combination_method,
coil_combination_method!,
flip_angle,
flip_angle!,
multiband_acceleration_factor,
multiband_acceleration_factor!,
pulse_sequence,
pulse_sequence!,
scanning_sequence,
scanning_sequence!,
sequence_variant,
sequence_variant!,
sequence_name,
sequence_name!,
nonlinear_gradient_correction,
nonlinear_gradient_correction!,
event_onset,
event_onset!,
event_duration,
event_duration!,
trial_type,
trial_type!,
response_time!,
stimulus_file,
stimulus_file!,
event_marker,
event_marker!,
event_description,
event_description!,
calmax,
calmax!,
calmin,
calmin!,
magic_bytes,
magic_bytes!,
nshots,
nshots!,
parallel_reduction_factor_in_plane,
parallel_reduction_factor_in_plane!,
parallel_acquisition_technique,
parallel_acquisition_technique!,
partial_fourier,
partial_fourier!,
partial_fourier_direction,
partial_fourier_direction!,
phase_encoding_direction,
phase_encoding_direction!,
effective_echo_spacing,
effective_echo_spacing!,
total_readout_time,
total_readout_time!
Copy link

Choose a reason for hiding this comment

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

I feel that this is a large number of names to export, especially given

  • A lot of these seem to be trivial wrappers around an internal dictionary-like interface.
  • There's a large number of other attributes which aren't covered here, and achieving full coverage of the very large number of DICOM attributes seems impractical.

Did you consider a more dictionary-like interface for some of these attributes? For example, perhaps allowing syntax such as

metadata(x).manufacturer

which is possible to make type stable if done carefully

(Achieving x.manufacturer is possible and tantalizing but likely undesirable due to the need to "steal" getproperty on some non-concrete supertype of x.)

I feel there may be two rough categories of metadata:

  1. Metadata which is directly required for working with the pixel data. For example, pixel spacings and coordinate systems.
  2. Metadata which is somewhat irrelevant or only indirectly relevant, such as the institution name.

The first type is a more limited set of metadata and we may plausibly get a minimal required set of functions for accessing this like ImageCore. The latter type of metadata is fairly open ended and I'd suggest it's better handled with some kind of dictionary like lookup. Does the images ecosystem also have a similar distinction?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a large number of other attributes which aren't covered here, and achieving full coverage of the very large number of DICOM attributes seems impractical.

I agree. That's why I've focused on what's in BIDS. It's still a lot but theoretically this can be broken up into smaller packages if it's found to be useful (eg., BIDSImage, BIDSEeg, BIDSData, etc).

The latter type of metadata is fairly open ended and I'd suggest it's better handled with some kind of dictionary like lookup.

A large portion of this is in fact a wrapper around a dictionary lookup but makes it type stable. JuliaImages uses properties to retrieve a dictionary with keytype == String and valtype == Any. ImageMeta has a nice some nice syntax that allows img[::String] to search the underlying property structure. However, that would restrict everything to have ImageMeta as the top level wrapper where it might not be appropriate (e.g., non arrays such as graphs, fiber tracks, etc.).

One example where this could be immediately useful is if you look at MRIReco.jl where there are distinct structures for raw acquisitional data, sequence parameters, etc. But very little of that is needed in NIfTI files. There's some overlap but it's not enough to agree upon a single structure for say acquisition parameters. This example doesn't even account for GIFTI, CIFTI, BED , etc. where there the data isn't an image or is a mesh instead.

All that being said, I'm certainly open to a better solution. I just don't think there will ever be a single structure everyone can agree on which as you pointed out means using a dictionary. If there's another way of guaranteeing that I get Float64 everytime I call img["EchoTime"] then we could just require that syntax.

Copy link

Choose a reason for hiding this comment

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

If there's another way of guaranteeing that I get Float64 everytime I call img["EchoTime"] then we could just require that syntax.

Sure. Very basic sketch:

struct Metadata
     d::Dict{Symbol,Any}
end

function Base.getproperty(m::Metadata, s::Symbol)
    if s === :EchoTime
        getfield(m, :d)[:EchoTime]::Float64 # Inferrable
    else
        getfield(m, :d)[s]  # Fallback (uninferrable)
    end
end
julia> m = Metadata(Dict(:EchoTime=>1.0, :Foo=>"hi"))
Metadata(Dict{Symbol,Any}(:EchoTime => 1.0,:Foo => "hi"))

julia> m.EchoTime
1.0

julia> m.Foo
"hi"

julia> @code_warntype (m->m.EchoTime)(m)
Variables
  #self#::Core.Compiler.Const(var"#15#16"(), false)
  m::Metadata

Body::Float64
1 ─ %1 = Base.getproperty(m, :EchoTime)::Float64
└──      return %1

julia> @code_warntype (m->m.Foo)(m)
Variables
  #self#::Core.Compiler.Const(var"#17#18"(), false)
  m::Metadata

Body::Any
1 ─ %1 = Base.getproperty(m, :Foo)::Any
└──      return %1

Then provide a generic function metadata(x) which for any x should return something with the same property-like interface as Metadata (it could literally be a concrete Metadata, or some other package-specific type).

Copy link

@c42f c42f Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Clearly, that type assertion for ::Float64 is a simplification and you may want to dispatch to extra logic there.

The main point is that getproperty can be used to look up properties in a flexible backing dictionary and certain blessed property names can be treated in a special way which makes them inferrable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that approach a lot. I would need to create a unique array type that reimplements all of the dictionary traits in JuliaImages that using String as the keytype though. That or convince JuliaImages to change to a Symbol keytype.

Copy link

Choose a reason for hiding this comment

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

Cool, it will be interesting to see how it goes.

Note also that Metadata in my sketch could be implemented with actual fields for a subset of "expected" metadata, and a dict as a fallback for the rest. This would avoid the dict lookup for the set of blessed attributes, and would therefore be more efficient provided x keeps the Metadata in this format when it's constructed so that no work is required to compute metadata(x). Of course, you may want to call it MRIMetadata or some such, given most of these fields are very MRI-specific. Other types of x would have their own type with the same API but different blessed attributes.

struct MRIMetadata
     echo_time::Union{Float64,Nothing}
     # ...
     d::Dict{Symbol,Any}
end

Copy link
Member Author

Choose a reason for hiding this comment

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

This leads me to a somewhat ignorant question. When I call sizeof on your structure it doesn't seem to matter if I use Nothing or Float64. Would it make sense to do something like this instead:

struct MRIMetadata{ET<:Union{Float64,Nothing}}
    echo_time::ET
   d::Dict{Symbol,Any}
end

This would prevent allocating memory for things that only have a couple fields that aren't nothing. I know this may be trivial right now, but if we want to have this metadata be flexible enough to work with something like individual fibers tracking algorithms, then it could get pretty expensive.

Copy link

@c42f c42f Dec 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think that parameterizing MRIMetadata on the presence or absence of metadata fields would be unnecessarily hard on the compiler and confer no practical advantage (it will need to create a new type each time the metadata changes). If you're worried about storage, you could go with a pure-Dict based solution instead. That would be simpler anyway so it might be for the best. Dict lookup is quite fast and is unlikely to be done in any inner analysis loop.

(Besides this, I would have imagined the relative sizeof(MRIMetdata) to sizeof(image_data) could easily be 1:1000 in which case it's not worth worrying about.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that parameterizing MRIMetadata on the presence or absence of metadata fields would be unnecessarily hard on the compiler and confer no practical advantage

I should probably clarify. I imagined that most users wouldn't ever construct these and that it would serve as a common way of organizing and accessing metadata for those actually creating packages.

struct Metadata{ET<:Union{Float64,Nothing}}
    echo_time::ET
   d::Dict{Symbol,Any}
end

const EmptMetadata =Metadata{Nothing}

This would allow the person implementing an EEG file reader to not worry about echo time but still use the same interface. As you said, it may be simpler to just go with a dictionary.

I would have imagined the relative sizeof(MRIMetdata) to sizeof(image_data) could easily be 1:1000 in which case it's not worth worrying about.

I agree with this in so far as it pertains to images, but this should be applicable outside of MRI as well. For example, we could have a small connectome composed of a 13x13 array across 10,000 subjects. This is definitely the extreme but I'd hate to paint myself into a corner early on.

Copy link

Choose a reason for hiding this comment

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

If it's worrying you, I'd suggest go for the Dict. It will be less code anyway :-)

But I also don't think you're painting yourself into a corner too much by adopting one implementation or another. The public interface to this stuff will be getproperty, which is allowed to present the metadata as if it came from an actual field. But whether there's an actual field present is an implementation detail.

I suspect what's most important / potentially annoying here is to settle on appropriate names for the metadata fields because these are very much the public interface. For example echo_time vs EchoTime. Probably the only viable thing is to stick with the original field names as defined in the BIDS standard, even though they conflict with the Julia naming conventions.



include("neuroproperty.jl")
include("properties.jl")

end
86 changes: 86 additions & 0 deletions src/neuroproperty.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
struct MissingProperty end
const MProperty = MissingProperty()

check_type(x, f, val, default) where {X} = _check_type(x, f, val)
check_type(x, f, ::MissingProperty, default) = _check_type(x, f, default)

_check_type(x, f::Function, default) = __check_type(x, f(x), default)
_check_type(x, ::Type{T}, default) where {T}= __check_type(x, T, default)
__check_type(x, ::Type{T}, f::Function) where {T} = __check_type(x, T, f(x))
__check_type(x, ::Type{T}, val) where {T} = val isa T ? val : T(val)::T


setter_error(x, n) = error("$(typeof(x)) does not have `properties` method. Cannot set $(n) property.")

_default_getter_def(property_name::String) = "Returns the `$(property_name)` property of `x`."
_default_setter_def(property_name::String) = "Sets the `$(property_name)` property of `x` to `val`."

function compose_definition(method_string, method_def, property_def)
return string(method_string, "\n\n", method_def, "\n\n", property_def)
end
function compose_definition(method_string, method_def, ::Nothing)
return string(method_string, "\n\n", method_def)
end

function compose_getter_string(method_name::Symbol, ::Type{T}) where {T}
return string(method_name, "(x) -> ", T)
end
compose_getter_string(method_name::Symbol, ::Function) = string("\t\t", method_name, "(x)")

compose_setter_string(method_name::Symbol) = string("\t\t", method_name, "(x)")

function neuroproperty(
getter::Symbol,
property_name::String,
default_type,
default_value;
setter=Symbol(getter, :!),
getter_def=_default_getter_def(property_name),
setter_def=_default_setter_def(property_name),
property_def=nothing
)
return _neuroproperty(
getter,
setter,
property_name,
default_type,
default_value,
compose_definition(compose_getter_string(getter, default_type), getter_def, property_def),
compose_definition(compose_setter_string(setter), setter_def, property_def)
)
end


function _neuroproperty(
getter,
setter,
property_name,
default_type,
default_value,
getter_doc,
setter_doc
)
_getter = Symbol(:_, getter)
_setter = Symbol(:_, setter)

check_val = Symbol(:check_, getter)

@eval begin
$check_val(x, val) = NeuroCore.check_type(x, $default_type, val, $default_value)

@doc $getter_doc $getter(x) = $check_val(x, $_getter(ImageCore.HasProperties(x), x))
$getter(x::AbstractDict{String}) = $check_val(x, get(x, $property_name, NeuroCore.MProperty))

$_getter(::ImageCore.HasProperties{false}, x) = NeuroCore.MProperty
$_getter(::ImageCore.HasProperties{true}, x) = get(properties(x), $property_name, NeuroCore.MProperty)

@doc $setter_doc $setter(x, val) = $_setter(ImageCore.HasProperties(x), x, $check_val(x, val))
$setter(x) = $_setter(ImageCore.HasProperties(x), x, $check_val(x, NeuroCore.MProperty))
$setter(x) = setindex!(x, $check_val(x, NeuroCore.MProperty), $property_name)

$setter(x::AbstractDict{String}, val) = setindex!(x, $check_val(x, val), $property_name)

$_setter(::ImageCore.HasProperties{false}, x, val) = NeuroCore.setter_error(x, $property_name)
$_setter(::ImageCore.HasProperties{true}, x, val) = setindex!(properties(x), val, $property_name)
end
end
Loading