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

How much should we support time based traits? #112

Closed
Tokazama opened this issue Jan 11, 2020 · 22 comments
Closed

How much should we support time based traits? #112

Tokazama opened this issue Jan 11, 2020 · 22 comments

Comments

@Tokazama
Copy link
Contributor

I brought this up on Slack and thought it would be good to have a formal issue for it. We have some basic support for identifying the time axis in ImageAxes and we have nimages and assert_timedim_last. However, very common (and convenient) to have some additional methods for interacting with and describing time. Personally I'm interested in having:

  • sampling_rate: number of samples per second
  • duration: length of time the data describes
  • onset: first time point

I'm sure there could be others but at this point I think it would make the most sense to focus on traits/methods that are universally helpful for interacting with data that has a time axis.

Is this something we should have in JuliaImages and if so, should this be a part of ImageCore or ImageAxes?

@timholy
Copy link
Member

timholy commented Jan 11, 2020

I am of the growing opinion that we should only support what we can be sure of, and in this package there is no way to know whether an array even has a time dimension.

With this philosophy, the implementation of assert_timedim_last in this package has the wrong fallback: it should be an error, rather than a default-pass. I recognize that may be an indication that I'm using the wrong philosophy, but to current-me the name suggests that it provides a guarantee that the last dimension corresponds to time. Which you can't guarantee without an axis that has been flagged as being of the time type.

@Tokazama
Copy link
Contributor Author

Just to clarify, does that mean you think this package should only have functionality related to spatial and color components of images? Does this mean we should move time related functionality out of this package? I'm assuming if we did so we would still want whatever package that contains it as a dependency in Images.jl and other related packages.

@timholy
Copy link
Member

timholy commented Jan 11, 2020

I guess I'm saying this package should define "stubs" but that the default might be to throw an error. (I'm not sure I'm entirely happy with my previous choices.)

@Tokazama
Copy link
Contributor Author

So you mean something like...

"""
    sampling_rate

...
"""
sampling_rate

...and just let upstream define methods?

I think in an ideal world we would be able to depend on another package that is dedicated to time series analysis and defines these methods using AxisArrays or a generic interface we can just inherit from. However, I'm not aware of any well established package that fits this description (most are domain specific and we would be inheriting only the syntax at this point AFIK). From there I just assumed that images is work with space and time axes and that's my use case, so here I am. I'm pretty open to whatever people think is best as long as it doesn't mean waiting five months for a counsel of experts on time series analysis to agree on syntax.

@johnnychen94
Copy link
Member

johnnychen94 commented Jan 12, 2020

Could it be defining these symbols in ImagesAPI, and then reexporting (and possibly extending) them from ImageCore?

@Tokazama
Copy link
Contributor Author

Could it be defining these symbols in ImagesAPI, and then reexporting (and possibly extending) them from ImageCore?

This was kind of my original intention when I suggested ImagesAPI. I figured there would be a lot very generic syntax we'd want common across the ecosystem but that the exact way it interacts with structures would be variable enough that we just need a common API package. However, I understand that's also a lot of what ImageCore is for right now.

I'm up for starting a PR wherever this feature is appropriate right now and if it seems reasonable I could also take care of some of the rough spots in the syntax/API with stuff like assert_timedim_last.

@johnnychen94
Copy link
Member

johnnychen94 commented Jan 13, 2020

If you need this feature eagerly, perhaps a better solution is to add them to another repository (say VideoCore.jl :P ) you take control of. When codes are stabilized and when we're realizing they're another core of the ecosystem of the JuliaImages, that's probably the right time to merge these two packages. But by then we might just want to keep two cores.

The issue I see is that some scattered traits in ImageCore.jl aren't enough to support the whole images processing tasks of time series, while you don't have the bigger plan in your mind right now (am I interpreting it right?) I think that's @timholy was worried about.

@Tokazama
Copy link
Contributor Author

If you need this feature eagerly, perhaps a better solution is to add them to another repository (say VideoCore.jl :P )

I'm totally open to that and was actually going to just define my own stuff in another package but @baggepinnen brought up temporal filtering on slack. I thought others might be interested in being able to reference a common set of methods.

some scattered traits in ImageCore.jl aren't enough to support the whole images processing tasks of time series

I completely agree with this but I don't think it's necessarily the issue with including methods that reference the time domain. Most methods in ImageCore.jl don't support the whole of any processing task. Most are just needed across a wide variety of tasks and make sense to have in the core package, right? However, I can see how hosting core methods concerning time may be outside of the scope of JuliaImages.

while you don't have the bigger plan in your mind right now

I don't have bigger plans for a dedicated time series package in JuliaImages but I always have bigger plans ;)

@timholy
Copy link
Member

timholy commented Jan 15, 2020

Sorry for the delay, I'm back to this now.

Having these as traits might make sense, but I'd prefer something that composes more broadly. Already timeaxis gets you almost all the way there. Setup:

using ImageAxes, Unitful
using AxisArrays: AxisArray
const mm = u"mm"
const s = u"s"
img = AxisArray(rand(3, 3, 20), (:y, :x, :time), (1.2mm, 0.8mm, 0.15s), (0mm, 10mm, 0.8s))

Then, for your three items in your top post:

sampling_rate(img) = 1/step(timeaxis(img).val)
duration(img) = (ax = timeaxis(img); last(ax.val) - first(ax.val))
onset(img) = first(timeaxis(img).val)

When things are this easy, there's a bit of a downside in a huge explosion of exported names. Can we first ask what needs to be changed to make timeaxis immediately useful? To me, the most obvious is that the ax.val construct is ugly; most likely we should support first, last, and step on Axis objects.

@Tokazama
Copy link
Contributor Author

I really like the idea of taking this back to some fundamental interactions on Axis. I'll open an issue over there and if it seems like people are happy with the idea I'll make a PR.

@timholy
Copy link
Member

timholy commented Jan 27, 2020

Now that JuliaArrays/AxisArrays.jl#176 is released, should this be closed?

@Tokazama
Copy link
Contributor Author

Tokazama commented Jan 27, 2020

tl;dr: If the answer we've converged on here is that JuliaImages should only worry about an axis interface that provides sufficient information to implement this, then it should be closed.

I still think there is a place for specific time based methods (although it might not be here). While your previously proposed methods solve the majority of cases, there are instances where sampling isn't continuous along the axis. There are also a bunch of other things that I haven't brought up that might be useful, like timestamps.

Time is a pretty fundamental component of many non image related analyses and I think we could leverage a lot of other efforts. There seems to be a lot of interest in Julia for getting behind some sort of standard for this (see this discourse thread for reference) so I think the long term plan should be depending on some other effort. In order for this to work we'd need to use a generalizable array approach like AxisArrays (instead of a structure with a time axis referenced like TimeSeries.jl does). I would love to get involved in those conversation if only to convince people to adopt a standard like AxisArrays but this last effort has me convinced that we're not ready for this.

Perhaps once there's a stronger consensus on how to approach named dimensions and labeled axes I could revisit this, but for now I think this is the best we're going to do.

@timholy
Copy link
Member

timholy commented Jan 22, 2021

@Tokazama, ImageCore 0.9 is coming quite soon, and I hope it will be the last 0.x release before 1.0 (though it's not a crisis if it isn't). Anything to report on your quest to improve traits?

@Tokazama
Copy link
Contributor Author

I was going to wait to ping you until I had working examples for SpatioTemporalTraits.jl, but you might have some comments on it at this point. I've got it down to solely depending on ArrayInterface.jl so that people will be less apprehensive about buying into it.

There are two things that I'm trying to get in line before getting it published

  1. I'm working on a PR right now to reduce the number of generated functions in ArrayInterface.jl. It may have some breaking changes, so I want to get that through before pushing ArrayInterface as a dependency for other packages.
  2. I'm toying with the idea of adding a StaticSymbol to ArrayInterface.jl so that changes in the compiler's ability to infer through constant propagation don't break things between minor versions.

I'm hoping to submit that PR to ArrayInterface today. Throughout this next week I'm going to try to integrate changes in SpatioTemporalTraits.jl and AxisIndices.jl so that working examples and docs can go forward. The next steps will be building up CPU methods (e.g., LoopVectorization based libraries) and getting the GPU packages on board with ArrayInterface. With all of that in place time spatial and temporal traits should be generically supported throughout most of Julia ecosystem.

@timholy
Copy link
Member

timholy commented Feb 9, 2021

#157 might become ImageCore 0.9. Happy to hold the release for a bit if you think it best, but if it will still be a while then let's do traits in ImageCore 0.10.

@Tokazama
Copy link
Contributor Author

Tokazama commented Feb 10, 2021

I'm assuming it will be at least a week before I can work out all the bugs so that it isn't too disruptive to JuliaImages. I'm trying to get ArrayInterface.jl to work seamlessly between arrays in Base and any new ones we might need in this PR JuliaArrays/ArrayInterface.jl#120. Propagating traits through ReshapedArrays is proving a bit difficult.

@timholy
Copy link
Member

timholy commented Feb 10, 2021

I'm happy to hold off for a week. If a week stretches into a month, then we should probably release and then do a second one with the traits.

@timholy
Copy link
Member

timholy commented Feb 10, 2021

Re JuliaArrays/ArrayInterface.jl#120 (comment), you should definitely be using reinterpret(reshape, T, A). It makes everything better.

@Tokazama
Copy link
Contributor Author

The problem I'm having is that reshaping takes away all of the static info about size and offsets. Given static sizes I think I could just infer what the new shape is based on the the change in element size.

@timholy
Copy link
Member

timholy commented Feb 10, 2021

That's what reinterpret(reshape, T, A) fixes. There's no ReshapedArray wrapper at all.

@johnnychen94
Copy link
Member

johnnychen94 commented Sep 7, 2021

I prefer to close this in favor of JuliaImages/Images.jl#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.

@timholy
Copy link
Member

timholy commented Sep 8, 2021

Not sure I agree (a lot of the code in https://github.com/HolyLab uses at least time-based traits), but now that SpatioTemporalTraits seems to have been fleshed out I think discussion should move there.

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