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

bugfixes, updates, more tests #23

Merged
merged 1 commit into from
Oct 8, 2020
Merged

Conversation

ranocha
Copy link
Member

@ranocha ranocha commented Oct 8, 2020

This closes #20 and simplifies the code. Additionally, I've updated the Travis settings and moved to Project.toml etc. Moreover, I've fixed the following bug found by @erik-f.

julia> using EllipsisNotation

julia> b = reshape(1:8, 2, 2, 2);

julia> b[.., 1]
2×2 Array{Int64,2}:
 1  3
 2  4

julia> b[.., :, 1]
ERROR: ArgumentError: too many indices provided
Stacktrace:
 [1] fillcolons(::Tuple{Colon,Colon,Colon}, ::Tuple{}, ::Tuple{Colon,Int64}) at ~/.julia/packages/EllipsisNotation/jWHb3/src/EllipsisNotation.jl:13
 [2] fillcolons at ~/.julia/packages/EllipsisNotation/jWHb3/src/EllipsisNotation.jl:15 [inlined] (repeats 3 times)
 [3] fillcolons at ~/.julia/packages/EllipsisNotation/jWHb3/src/EllipsisNotation.jl:10 [inlined]
 [4] to_indices at ~/.julia/packages/EllipsisNotation/jWHb3/src/EllipsisNotation.jl:19 [inlined]
 [5] to_indices at ./indices.jl:321 [inlined]
 [6] getindex(::Base.ReshapedArray{Int64,3,UnitRange{Int64},Tuple{}}, ::EllipsisNotation.Ellipsis, ::Function, ::Int64) at ./abstractarray.jl:1060
 [7] top-level scope at REPL[4]:1

The example from #20:

julia> using BenchmarkTools, EllipsisNotation

julia> u = rand(1, 2, 3);

julia> @benchmark view($u, $1, $..) # that's okay
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     1.889 ns (0.00% GC)
  median time:      1.907 ns (0.00% GC)
  mean time:        1.916 ns (0.00% GC)
  maximum time:     18.367 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1000

julia> u = rand(1, 2, 3, 4);

julia> @benchmark view($u, $1, $..) # that was slow and allocated a lot...
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     2.305 ns (0.00% GC)
  median time:      2.361 ns (0.00% GC)
  mean time:        2.455 ns (0.00% GC)
  maximum time:     19.384 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1000

julia> @benchmark view($u, $1, $:, $:, $:) # that's fast again
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     2.516 ns (0.00% GC)
  median time:      2.728 ns (0.00% GC)
  mean time:        2.714 ns (0.00% GC)
  maximum time:     17.892 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1000

@sloede
Copy link

sloede commented Oct 8, 2020

This is crazy! This package has now only 4-5 active lines of code left 🙀

@ChrisRackauckas
Copy link
Member

Awesome, ntuple worked! It needed v1.0's interprocedural optimization I guess. Nice!

@ChrisRackauckas ChrisRackauckas merged commit 1ceca32 into SciML:master Oct 8, 2020
@ranocha
Copy link
Member Author

ranocha commented Oct 8, 2020

Thanks. Would be nice to have a new release using this.

@ChrisRackauckas
Copy link
Member

Yup just did. I think it's appropriate to be a 1.0.

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.

.. becomes slow and allocates for 4 or more dimensions
3 participants