Optimize ArbitraryMotion (continuation) #408
Conversation
There was a problem hiding this comment.
Hi, please add the changes introduced by @rkierulf (currently in dev).
The SimpleMotion is having some weird issues with Metal, so we are skipping those tests (not sure if this is already in dev).
If this is not solved we should remove the simple motion example, as tutorials should work everywhere.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #408 +/- ##
==========================================
+ Coverage 89.19% 90.45% +1.26%
==========================================
Files 49 51 +2
Lines 2803 2796 -7
==========================================
+ Hits 2500 2529 +29
+ Misses 303 267 -36
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
I have just included the changes by @rkierulf in #406. I have removed adapts for ArbitraryMotion and NoMotion, but I did nothing to SimpleMotion, so these problems with Metal would still remain. I do not know a way of solving this, and I cannot test it since an Apple computer is needed, right? |
|
Related #407. |
Can you point to the issue when you submit it? I think it is a reasonable solution if we don't use ranges and the performance is not impacted. At this point, I think it will be faster to look for other options like https://docs.sciml.ai/DataInterpolations/dev/#DataInterpolations.jl than fixing all the bugs with Interpolations.jl. This package also accepts non-uniformly sampled data. As the interface is simpler it would be easier to debug. But also has some problems evaluating GPU arrays. I wonder if just implementing this ourselves with KernelAbstractions.jl, just for linear interpolations, is better (like we discussed some time ago).
Hopefully, we will find an easy solution to this (even better if Interpolations.jl works). |
|
I did a lot of commits in order to get the simplest reproducible code that gives the error that we have.
See JuliaMath/Interpolations.jl#597. Last commit leaves everything like it was before:
But, even with this error, I would merge this PR soon anyway, since simulation speed for ArbitraryMotion, compared to master, has been significantly increased: We have also solved the errors with In the meantime, I am still investigating the reason for the error in the test and looking for better solutions for the interpolation. |
| id = similar(motion.dx, Ns); copyto!(id, collect(range(oneunit(T), T(Ns), Ns))) | ||
| t = similar(motion.dx, Nt); copyto!(t, collect(range(zero(T), oneunit(T), Nt))) |
There was a problem hiding this comment.
| id = similar(motion.dx, Ns); copyto!(id, collect(range(oneunit(T), T(Ns), Ns))) | |
| t = similar(motion.dx, Nt); copyto!(t, collect(range(zero(T), oneunit(T), Nt))) | |
| id = similar(motion.dx, Ns); id .= range(oneunit(T), T(Ns), Ns) | |
| t = similar(motion.dx, Nt); t .= range(zero(T), oneunit(T), Nt) |
Is id .= 1:Ns not enough?
Are the copyto!(collect( necessary? They are a little bit ugly.
There was a problem hiding this comment.
It looks like metal and oneAPI don't have this operation defined for ranges or something like that.
When assigning wit =. and a range, the following error appears (see this):
ERROR: LoadError: InvalidIRError: compiling MethodInstance for (::Metal.var"#broadcast_linear#202")(::Metal.MtlDeviceVector{Float32, 1}, ::Base.Broadcast.Broadcasted{Metal.MtlArrayStyle{1, Metal.MTL.Private}, Tuple{Base.OneTo{Int64}}, typeof(identity), Tuple{Base.Broadcast.Extruded{StepRangeLen{Float32, Float64, Float64, Int64}, Tuple{Bool}, Tuple{Int64}}}}) resulted in invalid LLVM IR
From here, it looks like =. is defined for assigning scalars, but I think it's not the case for ranges.
There was a problem hiding this comment.
Ok, I think submitting a few issues to Metal/OneAPI would be good. I don't know what the MWE is, @rkierulf says these work
julia> using Metal
julia> A = ones(Float32, 4)
4-element Vector{Float32}:
1.0
1.0
1.0
1.0
julia> B = convert(MtlArray,A)
4-element MtlVector{Float32, Private}:
1.0
1.0
1.0
1.0
julia> B .= range(1,4) # Error 1: id .= range(oneunit(T), T(Ns), Ns)
4-element MtlVector{Float32, Private}:
1.0
2.0
3.0
4.0
julia> B .+ B' # Error 2: xt, yt, zt = x .+ 0*t, y .+ 0*t, z .+ 0*t
4×4 MtlMatrix{Float32, Private}:
2.0 3.0 4.0 5.0
3.0 4.0 5.0 6.0
4.0 5.0 6.0 7.0
5.0 6.0 7.0 8.0- Error 1:
id .= range(oneunit(T), T(Ns), Ns) - Error 2:
xt, yt, zt = x .+ 0*t, y .+ 0*t, z .+ 0*t
KomaMRIBase/src/datatypes/phantom/motion/simplemotion/_utils.jl
Outdated
Show resolved
Hide resolved
Co-authored-by: Carlos Castillo Passi <[email protected]>
|
I think all the comments are solved.
|
Continuation of #396