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

Handling Image traits for AxisArrays successor #815

Closed
Tokazama opened this issue Jul 29, 2019 · 10 comments
Closed

Handling Image traits for AxisArrays successor #815

Tokazama opened this issue Jul 29, 2019 · 10 comments
Labels
api API changes enhancement Adds a new feature.

Comments

@Tokazama
Copy link

Tokazama commented Jul 29, 2019

So there is a secret coup to overthrow AxisArrays (see here). I think there's a way to easily support both throughout the transition without disrupting Images.

If we use more trait based dispatch for both the axis components and the metadata components of objects then integration is as straitforward as:

"""
    HasProperties{T}
"""
struct HasProperties{T} end

HasProperties(::T) where T = HasProperties(T)
HasProperties(::Type{T}) where T = HasProperties{false}()
HasProperties(::Type{<:ImageMeta}) = HasProperties{true}()

spatialproperties(img::T) where T = spatialproperties(HasProperties(img), img)

spatialproperties(::HasAxes{true}, img::T) where T = @get img "spatialproperties" ["spacedirections"]
spatialproperties(::HasAxes{false}, img::T) where T = ["spacedirections"]

We could similarly use HasAxes from AxisArrays to accomplish the same thing.

some_function_requiring_names(img::T) where T = some_function_requiring_names(HasAxes(T), img)

some_function_requiring_names(::HasAxes{true}, img::T) where T = some_function_requiring_names(axisnames(img), img)
some_function_requiring_names(::HasAxes{false}, img::T) where T = default_output

I know this would be a fair bit of work but it would be aimed at make no changes to the actual API, functionally, or dependencies. It would just allow other packages to more easily integrate with the Image ecosystem.

If this seems acceptable I would be willing to move forward with this and make some pull requests.

@johnnychen94
Copy link
Member

johnnychen94 commented Aug 3, 2019

@Tokazama Looks like there's some error here, could you please explain how this works?

HasProperties(::T) where T = HasProperties(T)
...
if HasProperties(img)
   @get img "spatialproperties" ["spacedirections"]
...

I'd really like to give some meaningful comments, but I still don't quite understand the advantages of this over the current method, i.e., dispatching on img

some_function(img::AbstractArray)
some_function(img::ImageMeta)

I guess we're thinking of two different usages?

Is it because you want to have more fined specification on ImageMeta's properties?

@Tokazama
Copy link
Author

Tokazama commented Aug 3, 2019

Sorry, I put that together pretty quickly. I fixed the first example to be more like what would be implemented.

If we assume some_function requires axis names then we have two functions:

some_function(img::AbstractArray)
some_function(img::ImageMeta{T,N,<:AxisArray}) where {T,N}

The first implements the fall back given a lack of axis names while the second has the desired implementation. If there is a new array type that defines axisnames then it still has to redefine the entire function with the desired behavior.

In other words, using something like HasAxes would allow:

some_function(img) = some_function(HasAxes(img), img)
some_function(::HasAxes{true}, img) = _some_function(axisnames(img), img)
some_function(::HasAxes{false}, img) = default_output

@Tokazama
Copy link
Author

Tokazama commented Aug 3, 2019

I'm not recommending this as something that should happen just because I'd personally prefer it (although I do), but because the current design of Images.jl would either need a change like I'm suggesting or would need to be entirely rewritten for the up and coming AxisArrays.jl replacement.

@johnnychen94
Copy link
Member

Personally, I also like this idea because it's more dynamic while providing the same functionality.

A potential issue is the maintainability issue introduced by this, if the codes are not well-documented or organized, it would be a nightmare for others to follow up...

In the meantime, I think we also need to think about making AxisArray.jl a real addon packages to Images.jl as discussed in #792 (comment)

@Tokazama
Copy link
Author

Tokazama commented Aug 3, 2019

I think the biggest problem would really be converging on the preferred names for functions (for example axisnames may eventually be replaced with names). But besides that it shouldn't break anything in the interim and should be fairly straight forward.

Given the current state of Images.jl it seems like the only traits that could be made would be something to ensure access to properties, axisnames, and AxisArrays.axes. Everything else should just use these to ensure stability over time.

@timholy
Copy link
Member

timholy commented Aug 12, 2019

This seems like a good goal. I guess the question is whether we should return HasAxes{true}() or just the axes themselves. But I think your proposal is the right one because returning the axes might force one to implement the method twice.

@Tokazama
Copy link
Author

Update on this. This https:/Tokazama/AxisIndices.jl will be registered soon. It should allow us to use NamedDims.jl and the nice indexing from AxisArrays + some other nice features. If you go here https:/Tokazama/NamedIndicesMeta.jl I have already written the code to make this all compatible with Julia Images (or at least ImageAxes.jl). It's passing all the pertinent tests that ImageAxes has on my computer locally, but I'm not sure it's the best place (or naming) for the final code that glues everything together.

@Tokazama
Copy link
Author

I'd like to gage interest and get feedback on a some developments relevant to this issue. I currently have 4 experimental modules in AxisIndices (AxisIndices.SptialDims, AxisIndices.AxisIndices.ObservationDims, AxisIndices.TimeDims, and AxisIndices.ColorDims). I'm pulling out TimeDims and registering it as TimeAxes which should effectively solve this. I'm trying to formulate how to pull the other modules out into appropriate packages because I think they could help solve a lot of problems. To name a few:

  • It makes maintaining other packages like ImageCore a lot easier. Each module I mentioned recreates traits from ImageCore but uses a single macro (here) to accomplish this resulting in very consistent syntax (addressing this issue) with generic implementations. For example, all the code for indices_spatial is implemented with a single method.
  • I'm working on OffsetAxes when I have spare time. It's actually pretty easy to implement things like OffsetArrays, IdentityRanges, and centered axes because indexing is specified at each axis (I'm just trying to ensure it passes all the tests created in other relevant packages). This should solve a lot of issues (potentially including some mentioned here).
  • Although the traits in each module I've mentioned are simple there's a lot more that can be done with them. This could avoid isolating these packages from other parts of the Julia ecosystem (as @timholy pointed out being an issue here in the final bullet).

I'm open to any feedback but I'm specifically am concerned with the following:

  • What do others think about the syntax I've chosen for the traits (easy to read in the docs here). I know it would create a bunch of breaking changes if implemented in ImageCore but I think it's the only way to address my previous comment concerning this.
  • Should there be individual SpatialAxes and ColorAxes packages, or do you think there's a more appropriate place for these? (assuming you think there is a place for these).

@johnnychen94
Copy link
Member

@Tokazama I think this issue can be safely closed in favor of #908? It's a little bit unclear to me what concrete plans are left in this issue.

@Tokazama
Copy link
Author

Tokazama commented Apr 3, 2021

Yes! Sorry for the slow response. The ebbs and flows of human subject research can be an unpredictable consumption of time.

@Tokazama Tokazama closed this as completed Apr 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api API changes enhancement Adds a new feature.
Projects
None yet
Development

No branches or pull requests

3 participants