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

Better axis support via name traits #908

Closed
Tokazama opened this issue Aug 4, 2020 · 10 comments
Closed

Better axis support via name traits #908

Tokazama opened this issue Aug 4, 2020 · 10 comments
Labels
enhancement Adds a new feature.

Comments

@Tokazama
Copy link

Tokazama commented Aug 4, 2020

I think this may be a good time for me to begin formally introducing what I've been working on. It's not coming in the form of a PR right now b/c it's very breaking and I needed some of the features for NeuroCore right now, so a lot of this can be accessed right now through sub-modules in NeuroCore. I also haven't heavily discussed most of the details b/c I was afraid the individual details seemed only trivially helpful despite introducing breaking changes, but as a whole I think it solve a lot of problems. What I'm proposing would replace ImageCore with two separate modules (ColorChannels and SpatialAPI), implement a bunch of syntactic changes, change dependencies, and ultimately move more non-image specific functionality into a more broad community.

Replacing ImageAxes, ImageMetadata, and other array types with something generic

I'm starting with this because one of the main goals throughout all of this was to get non-image specific stuff in a position to receive maintenance and compatibility across the wider Julia community. I won't waste anyone's time going over AxisArrays' issues, so I'll just keep it short and sweet. I think this is solved by using NamedDims.jl and AxisIndices.jl. Furthermore, I think that the approach in AxisIndices for managing fancy indexing stuff solves a bunch of other problems. I'll just introduce the following type as the characteristic type for accomplishing all of this, NameMetaAxisArray.

First, ImageAxes.jl is mostly just AxisArrays.jl with some time functionality (plus a couple other things). However, there still is no dedicated API for time methods in Julia. So I created TimeAxes.jl which has the functionality provided by ImageAxes.jl, but also provides a lot of the tools available in TimeSeries.jl. It also makes AxisArray compatible with FFTW.jl and AbstractFFTs.jl.

Second, changing stuff at the axis level fixes a lot of stuff. If you want to remap an indexing arguments to indices AxisIndices provides OffsetAxis, IdentityAxis, and CenteredAxis to solve this. The features aren't 1-1 replacements of other packages because there are some places where I'm not sure of the original packages have universally pleased people (e.g., I haven't overloaded similar(::AbstractArray, ::Vararg{Union{<:AbstractAxis,AbstractUnitRange}}) because it's caused some ambiguities for others when using OffsetArrays). One consequence of doing this at the axis level instead of the array is that functionality that is tested with AxisArray now works with offset axes. It also allows compatibility with static sizes because each axis can independently be made static.

Third, the metadata API basically just requires that you call metadata(x) to extract the metadata. You can use x.property to call getproperty(metadata(x), property) if it's an appropriate type. I've made this compatible with AbstractDict{Symbol,Any} metadata too.

ImageCore -> ColorChannels and SpatialAPI

Biggest changes here are making the syntax consistent and making it compatible with NamedDims.jl and AxisIndices.jl (i.e. issue #767 ). These modules are currently available at NeuroCore.ColorChannels and NeuroCore.SpatialAPI. In the aforementioned issue I explained in more detail why I wanted to changes several method names (briefly, consistency of names and clearer meaning). I'm moving towards the BlueStyle guide mostly b/c the clear outline helps avoid as much time discussing naming things and it helps avoid issues like this where naming a single thing drags on for months.

  • Renames
    • spacedirections -> SpatialAPI.spatial_directions
    • spatialorder -> SpatialAPI.spatial_order
    • pixelspacing -> SpatialAPI.pixel_spacing
    • coords_spatial -> SpatialAPI.spatialdims
    • indices_spatial -> SpatialAPI.spatial_axes
    • size_spatial -> SpatialAPI.spatial_size
    • namedaxes -> AxisIndices.named_axes
    • rawview -> ColorChannels.raw_view
    • normedview -> ColorChannels.normed_view
    • colorview -> ColorChannels.color_view
    • scalesigned -> ColorChannels.scale_signed
    • takemap -> ColorChannels.take_map
    • colorsigned -> ColorChannels.color_signed
    • scaleminmax -> ColorChannels.scale_minmax
    • has_time_axis -> TimeAxes.has_timedim
    • timeaxis -> TimeAxes.time_axis
    • nimages -> TimeAxes.ntime

If you're curious about other functions that are provided you may be interested in glancing over some of this which uses a macro to help define methods related to formally named dimensions. Of course, this approach nicely enforces a consistent syntax (which I've probably already talked about too much). I also has some nice effects for testing. For example, I've made a point of copying the tests from each package I've been "replacing" and in several locations I've been able to newly achieve type stability. Since specially labelled dimensions for time, channels, and observations use this macro they all inherit improvements as I've gone through improving on old tests.

I should note that I haven't fully explored how far I can improve all of these things. For example, I've begun to achieve a lot what StructArrays.jl does through the use of StructAxis which maps types to each element along an axis. This could be useful for mapping color types along a given axis, but I haven't specifically developed this yet. I also have some unpublished code for applying padding at each individual axis, but haven't had the time to fully develop it. There's a lot of other things I could mention but this is probably the most readily applicable to JuliaImages.

Why break everything just for improved syntax and updated dependencies?

I understand that this is a lot and it's fairly presumptuous for me to make a lot of breaking changes without discussing each of them, but I saw a lot of nails that I could solve with one hammer. For reference, here are some of the issues this starts to resolve:

  • Reorganizing/simplifying package hierarchy
  • Making more generic image traits
  • Consistent naming
    • #767
  • Where should we put time based methods
  • Ground work for more flexibly mapping channel dimensions to color elements
  • Methods that wrap an AxisArray in a new type should still be able to return dimension names and special axes
  • Generic support for other libraries
  • Convolutions should be easier to generically support with Flux's NNlib
  • A lot of problems are related to various ways of iterating over an array, because AxisIndices.jl is basically axis specific remapping and indexing this gives related issue a pretty clear path forward.
    • It was easy to make iterators with windows/pads/gaps/etc was simple to implement so a package specifically for window mapping could move out of JuliaImages and still be completely compatible
    • Performance issues related to iterators is no longer needs to be spread across OffsetArrays.jl, TiledIteration.jl

Of course there are other places we could improve things but the point is really that I think we could move forward on a lot of issues by taking this approach.

Other changes and potential calamities

This is mostly replacing stuff in ImageCore.jl so I've tried to ensure that any functionality used/created therein is still present. Here are some current problems and temporary work arounds.

  • Until I have time to get axis level padding put together I've made AxisIndices.PaddedViews so that library is still available despite replacing OffsetArrays.jl.
  • I haven't completely figured out how to work with the StreamingContainer type. I didn't see any code using it when I searched for it on JuliaHub, so I put recreating it on hold for now.
  • spatialproperties: Honestly, this should take an afternoon to implement at the most but I just haven't gotten around to it yet.
  • Right now there is a dependency on Graphics.jl in ImageCore.jl. I switched this to GeometryBasics.jl because it provides most of the same functionality plus some and provides a more direct interface to the broader plotting ecosystem. Then again this only is used to apply an array specific method of width and height which is type piracy, so perhaps this shouldn't be in ImageCore.jl at all.
  • I'm not supporting Julia 1.0. I thought about it but I really wanted to use ArrayInterface.jl so that from the very beginning things were moving towards generic support in the broader community. However, ArrayInterface.jl doesn't support v1.0.

Next Steps

I understand that dropping the current LTS isn't really a great proposition, so perhaps working towards having an intermediate ColorChannels.jl and SpatialAPI.jl package would be a good intermediate step while everything gets ironed out. I'd like this not to turn into some random guy yelling into the void of github issues here. So if there's any serious feedback or reservations about these changes I haven't addressed please tell me and I'll try to fix them.

@timholy
Copy link
Member

timholy commented Aug 6, 2020

Thank you for posting this, and sorry for the delay in responding. On balance I am quite supportive of this idea. We need to make some breaking changes to ImageCore and this looks like perfect motivation. Can you clarify what level you'd put these things? Currently ImageCore is the inner layer of the onion, basically the layer between color-handling and arrays, and any named axes are included in the next layer of the onion. However, now you seem to be proposing basing the trait system on stuff that already loads all the named stuff. So do we basically hop from "low level color stuff" to "full named array systems" with nothing in between? I'm not necessarily opposed to that, just trying to understand.

If we do this, you know those traits will have to move out of NeuroCore, right? People's allergy to using ImageFiltering because of the "Image" in the package name has convinced me that package names should be no more domain specific than strictly necessary, and neuro is far more narrow than image.

ArrayInterface.jl is a good dependency. Should probably move to JuliaArrays though.

I haven't completely figured out how to work with the StreamingContainer type. I didn't see any code using it when I searched for it on JuliaHub, so I put recreating it on hold for now.

Oh, since you do neuroimaging you are missing out by not using StreamingContainer. We use it all the time, though I guess all that code is lab-internal. Think lazy gaussian filtering and the like. You can pass a StreamingContainer to ImageView and see the filtered movie on-the-fly. That said, ImageView needs to be better about allowing contrast manipulations of non-AbstractArray objects.

@Tokazama
Copy link
Author

Tokazama commented Aug 6, 2020

If we do this, you know those traits will have to move out of NeuroCore, right? People's allergy to using ImageFiltering because of the "Image" in the package name has convinced me that package names should be no more domain specific than strictly necessary, and neuro is far more narrow than image.

I'd definitely expect them to move out of NeuroCore. The only reason they are there right now is because I needed them right now. It would be very unfortunate if neuroscientists alone ended up maintaining core functionality for manipulating colors and spatial registration!

However, now you seem to be proposing basing the trait system on stuff that already loads all the named stuff.

Although metadata, dimension names, and special indexing would be implemented outside of core image libraries the image specific components would still be located in a JuliaImages package. It make be helpful to look at how I implement observation dimensions. I essentially define a single method that makes a claim for what is considered an observation dimension and then a macro produces a bunch of methods specific to it. That means that in terms of how something like colors are implemented very little is changed. With a few exceptions, I've nearly just copied and pasted what's in ImageCore.jl. The main addition is using this for defining the channels instead of what's currently in ImageAxes. I think this might help with wrangling data into an appropriate form for things like color_view, but I haven't played with it too much yet. Right now this mainly ensures there's something like ImageAxes.colordim available.

It may be worth noting that this doesn't allow users to arbitrarily add names that can be considered "time" dimensions. I think it would be possible to change this to be more flexible, but if we're going through the trouble of specializing on the name we might not want people to start arbitrarily changing what dimensions can be considered time.

Spatial traits are pretty similar. It's still just the dimensions that aren't time or channels.

Oh, since you do neuroimaging you are missing out by not using StreamingContainer. We use it all the time, though I guess all that code is lab-internal. Think lazy gaussian filtering and the like. You can pass a StreamingContainer to ImageView and see the filtered movie on-the-fly. That said, ImageView needs to be better about allowing contrast manipulations of non-AbstractArray objects.

That does sound nice. I've been so focused on image-cognitive analyses in the last year that I haven't done much bare bones signal processing. I had bits and pieces working but there were some type stability issues. Maybe I'll take another look at it.

@Tokazama
Copy link
Author

Would a good next move be to make a something like a SpatialAPI.jl package so that this can move forward without breaking Images.jl?

@timholy
Copy link
Member

timholy commented Aug 26, 2020

How about a package called SpatioTemporalArrayTraits? We want to get time in on the fun too.

I declare open season on shaking things up. (I'm basically done with my invalidations work, finally.)

@Tokazama
Copy link
Author

I declare open season on shaking things up. (I'm basically done with my invalidations work, finally.)

Congrats!

How about a package called SpatioTemporalArrayTraits? We want to get time in on the fun too.

Including time stuff would actually be great! Just throwing out a couple more names for thought:

  • SpatioTemporalTraits
  • SpatioTemporalAPI
  • SpatioTemporalInterface
  • TimeSpaceTraits
  • TimeSpaceAPI
  • TimeSpaceInterface

@timholy
Copy link
Member

timholy commented Aug 26, 2020

I like the Traits and Interface ones in preference to the API ones. I could live with any of them.

I propose JuliaArrays as a hosting organization. Do you have admin rights? If not I can create an empty shell for you to populate once we decide on a name.

@Tokazama
Copy link
Author

I like the Traits and Interface ones in preference to the API ones. I could live with any of them.

I think SpatioTemporalTraits sounds the nicest, but I'm not overly attached to any one name.

I propose JuliaArrays as a hosting organization. Do you have admin rights? If not I can create an empty shell for you to populate once we decide on a name.

I think that would be a great place for this. I don't have admin rights so that would be very helpful. Thanks!

@timholy
Copy link
Member

timholy commented Aug 26, 2020

Sounds good. Let's wait 24 or so hours for anyone else to chime in with naming preferences and then I'll pull the trigger.

@timholy
Copy link
Member

timholy commented Aug 27, 2020

Here's your blank canvas: https://github.com/JuliaArrays/SpatioTemporalTraits.jl

Paint away!

@johnnychen94
Copy link
Member

I prefer to close this in favor of #978; I'm not very convinced that adding traits solve the problem of temporal image processing so I believe we need more meta-level discussions on how the design and on how we make the abstractions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adds a new feature.
Projects
None yet
Development

No branches or pull requests

3 participants