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

use .. from EllipsisNotation.jl #57

Merged
merged 4 commits into from
Jan 28, 2020

Conversation

mcabbott
Copy link
Contributor

Closes #18

@mcabbott
Copy link
Contributor Author

Test failure on 1.0 appears to be caused by this list not being empty:

julia> using Test, EllipsisNotation; detect_ambiguities(EllipsisNotation, Base, Core)
2-element Array{Tuple{Method,Method},1}:
 (getindex(A::AbstractArray, ::EllipsisNotation.Ellipsis) in EllipsisNotation at /Users/me/.julia/dev/EllipsisNotation/src/EllipsisNotation.jl:25, getindex(v::JSON.Parser.PushVector, i) in JSON.Parser at /Users/me/.julia/packages/JSON/d89fA/src/pushvector.jl:15)                        
 (getindex(A::AbstractArray, ::EllipsisNotation.Ellipsis) in EllipsisNotation at /Users/me/.julia/dev/EllipsisNotation/src/EllipsisNotation.jl:25, getindex(a::GenericArray, i...) in Test at /Users/osx/buildbot/slave/package_osx64/build/usr/share/julia/stdlib/v1.0/Test/src/Test.jl:1553)

I think the offending line is this:

Base.getindex(A::AbstractArray, ::Ellipsis) = A

This list is not empty on 1.3 either, but that didn’t cause an error. I’m not entirely sure why this sees the package JSON at all.

@mcabbott
Copy link
Contributor Author

JSON was being loaded by BenchmarkTools in my statup. On 1.0 the remaining ambiguity warning is from getindex(a::GenericArray, i...) defined by Test, which was must have been removed from later versions. I’ve simply disabled that test on 1.0 now.

@dlfivefifty
Copy link
Member

This looks fine for me, just bump the version number.

@mcabbott
Copy link
Contributor Author

Great! Is a minor increment enough, or should it go to 0.4?

@dlfivefifty
Copy link
Member

Definitely 0.4

@dlfivefifty dlfivefifty merged commit ba476b0 into JuliaMath:master Jan 28, 2020
@mcabbott mcabbott deleted the ellipsisnotation branch January 28, 2020 18:57
mcabbott pushed a commit to mcabbott/AxisArrays.jl that referenced this pull request Feb 4, 2020
@mcabbott
Copy link
Contributor Author

The downside of this change is that EllipsisNotation.jl now depends on ArrayInterface.jl:

https://github.com/ChrisRackauckas/EllipsisNotation.jl/blob/master/Project.toml

So this adds around 5000 lines of code, not 20. If this package wants to stay lightweight, then we should consider reverting this.

@timholy
Copy link
Member

timholy commented Sep 14, 2021

Yeah, that's not great. Let's deal with the issue over in SciML/EllipsisNotation.jl#25

@mcabbott
Copy link
Contributor Author

mcabbott commented Feb 7, 2022

This PR is reverted in #83, btw.

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.

Compatibility with EllipsisNotation
3 participants