Skip to content

Conversation

@devmotion
Copy link
Member

When I tried to use ArrayPartitions I discovered #11 and the closed issue #8 about type instabilities. However, there are still many type instabilities left - e.g. results of zeros(x), last(x), and all vector space operations such as multiplication and addition can not be inferred. Moreover, not in-place broadcasting does only work if the types are not changed by the applied function, as noted in #11.

Hence I tried to improve ArrayPartition and fix most of these things. This PR should fix #11 (all broadcasts should work in a type-stable fashion) and #17 (ArrayPartition is now a subtype of AbstractVector{T}). Often types could not be inferred since generators were used, hence I replaced these implementations with a call to a generated function that acts as a simple unrolled generator of ArrayPartition. Furthermore, I tried to get rid of most type instabilities. Since I could not achieve a type-stable version of e.g. similar(x, (Int64, Float64)) I changed this non-standard method of similar to similar(x, Int64, Float64) since then it can be made type-stable (the problem is

julia> typeof((Int64,Float64))
Tuple{DataType,DataType}

i.e. the type signature of the definition with a tuple contains less information).

I was wondering about the (unchanged) behaviour of ones(x): in particular it also works for units (which is good, I guess), although ones usually does not work for arrays of units - this leads to

julia> ones([1u"m", 2u"m"])
ERROR: DimensionError: m and 1 are not dimensionally compatible.
Stacktrace:
 [1] fill!(::Array{Quantity{Int64, Dimensions:{𝐋}, Units:{m}},1}, ::Int64) at ./multidimensional.jl:788
 [2] ones(::Array{Quantity{Int64, Dimensions:{𝐋}, Units:{m}},1}, ::Type{T} where T) at ./array.jl:261 (repeats 2 times)
julia> ones(ArrayPartition([1u"m", 2u"m"]))
RecursiveArrayTools.ArrayPartition{Quantity{Int64, Dimensions:{𝐋:{𝐋
2-element Array{Quantity{Int64, Dimensions:{𝐋
 1 m
 1 m

which seems a bit surprising. Moreover, I do not know whether it is reasonable to define recursive_one for ArrayPartitions by only considering the first partition. Maybe it should not be defined?

@ChrisRackauckas
Copy link
Member

One thing to note is that ArrayPartitions should be general enough to to handle heterogeneous arrays:

SciML/DifferentialEquations.jl#147

Heterogeneity due to units in second order ODEs is one common area where this is used and I'd like it to still work. The goal of course is the indexing and everything is type stable when there's no units, but in the units case it's fine if just broadcasting is type stable. I'll be keeping this in mind as I review.

@ChrisRackauckas
Copy link
Member

Wow, this looks fantastic. I'm surprised you got everything to infer so well. Impressive. Did you test it a second order ODE with units?

@devmotion
Copy link
Member Author

No, so far I just did inference tests on ArrayPartitions (as shown in the tests) and compared some timings and allocations with master. For most of the operations, benchmarks were almost equal to master (often one allocation less) but the added last and colon operations A[:] are improved.

@coveralls
Copy link

coveralls commented Aug 16, 2017

Coverage Status

Coverage increased (+15.6%) to 58.571% when pulling 5352af9 on devmotion:change_arraypartition into 85e6bd7 on JuliaDiffEq:master.

@codecov
Copy link

codecov bot commented Aug 16, 2017

Codecov Report

Merging #18 into master will increase coverage by 18.2%.
The diff coverage is 79.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #18      +/-   ##
==========================================
+ Coverage   42.93%   61.13%   +18.2%     
==========================================
  Files           3        3              
  Lines         184      211      +27     
==========================================
+ Hits           79      129      +50     
+ Misses        105       82      -23
Impacted Files Coverage Δ
src/array_partition.jl 78.57% <79.01%> (+36.31%) ⬆️
src/utils.jl 43.47% <0%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85e6bd7...d5b657c. Read the comment docs.

@devmotion
Copy link
Member Author

I tried to solve the example at SciML/DifferentialEquations.jl#147 (comment). After defining recursive_eltype for ArrayPartitions (same as recursive_one: only first partition is considered) and a small change to the creation of rate_prototype in OrdinaryDiffEq I successfully obtain a solution.

@coveralls
Copy link

coveralls commented Aug 16, 2017

Coverage Status

Coverage increased (+18.2%) to 61.137% when pulling d5b657c on devmotion:change_arraypartition into 85e6bd7 on JuliaDiffEq:master.

@ChrisRackauckas ChrisRackauckas merged commit 09e4ad1 into SciML:master Aug 16, 2017
@devmotion devmotion deleted the change_arraypartition branch August 16, 2017 07:51
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

Successfully merging this pull request may close these issues.

ArrayPartition broadcast() assumes input and output types are the same

3 participants