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

Backport 3.x: stack (Julia PR 43334) #779

Merged
merged 1 commit into from
Aug 22, 2022
Merged

Backport 3.x: stack (Julia PR 43334) #779

merged 1 commit into from
Aug 22, 2022

Conversation

ararslan
Copy link
Member

Backport of #777 to the release-3 branch.

Closes #778

@ararslan ararslan changed the title Add stack (Julia PR 43334) Backport 3.x: stack (Julia PR 43334) Aug 21, 2022
@bkamins
Copy link
Member

bkamins commented Aug 21, 2022

Thank you (although there seem to be some backporting issues) CC @mcabbott

@codecov
Copy link

codecov bot commented Aug 21, 2022

Codecov Report

Merging #779 (27ebabb) into release-3 (952300c) will increase coverage by 1.85%.
The diff coverage is 98.70%.

@@              Coverage Diff              @@
##           release-3     #779      +/-   ##
=============================================
+ Coverage      79.69%   81.54%   +1.85%     
=============================================
  Files              4        4              
  Lines            714      791      +77     
=============================================
+ Hits             569      645      +76     
- Misses           145      146       +1     
Impacted Files Coverage Δ
src/iterators.jl 62.50% <ø> (ø)
src/Compat.jl 81.77% <98.70%> (+2.10%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ararslan
Copy link
Member Author

Working on that now. The one I'm looking into currently is the lack of Iterators.peel but I think I have an idea...

@mcabbott
Copy link
Contributor

Never tried this with ancient Julia, but notice the same error on 1.7 "LoadError: cannot assign a value to variable Base.IteratorSize from module Compat" which is weird.

My first guess was that it relied on @constprop but the Compat version of that does seem to have been backported to this branch.

@ararslan
Copy link
Member Author

I fixed the IteratorSize one locally, that was an include order issue.

@ararslan ararslan force-pushed the aa/backport-777 branch 4 times, most recently from 16c70a6 to 1f8c312 Compare August 21, 2022 22:22
@ararslan
Copy link
Member Author

Alright, the one remaining failure now is on Julia 1.0, something is causing a MethodError where the test expects an ArgumentError. My computer won't run my Julia 1.0 binaries anymore so I'm not able to diagnose.

@mcabbott
Copy link
Contributor

mcabbott commented Aug 22, 2022

On 1.0 with this PR I get this error:

julia> stack([(1,2), (3,4,5)]; dims=1)
ERROR: MethodError: no method matching UnitRange(::Int64)
Closest candidates are:
  UnitRange(::T<:Real, ::T<:Real) where T<:Real at range.jl:257
  UnitRange(::AbstractUnitRange) at range.jl:857
Stacktrace:
 [1] iterate at ./generator.jl:47 [inlined]
 [2] _collect(::Base.OneTo{Int64}, ::Base.Generator{Base.OneTo{Int64},Type{UnitRange}}, ::Base.EltypeUnknown, ::Base.HasShape{1}) at ./array.jl:632
 [3] collect_similar at ./array.jl:561 [inlined]
 [4] map at ./abstractarray.jl:1987 [inlined]
 [5] _stack_size_check at /Users/me/.julia/packages/Compat/Qd33g/src/Compat.jl:1613 [inlined]
 [6] _dim_stack!(::Val{1}, ::Array{Int64,2}, ::Tuple{Int64,Int64}, ::Base.Iterators.Rest{Array{Tuple{Int64,Int64,Vararg{Int64,N} where N},1},Int64}) at /Users/me/.julia/packages/Compat/Qd33g/src/Compat.jl:1604

from this:

julia> x = (1,2,3);

julia> axes(x)  # is not a tuple
Base.OneTo(3)

julia> map(UnitRange, axes(x))
ERROR: MethodError: no method matching UnitRange(::Int64)

I'm slightly alarmed that no other tests notice that axes has this weird behaviour on tuples.

@ararslan
Copy link
Member Author

😳

@ararslan
Copy link
Member Author

Incredible, I fixed it on 1.0 and broke it everywhere else

@mcabbott
Copy link
Contributor

That looks right. I guess the inference tests don't like non-const _tuple_axes = axes.

Backport of PR 777 to the `release-3` branch.

(cherry picked from commit dce2f96)
@ararslan
Copy link
Member Author

Finally!

@ararslan ararslan merged commit 5cbf275 into release-3 Aug 22, 2022
@ararslan ararslan deleted the aa/backport-777 branch August 22, 2022 01:32
@bkamins
Copy link
Member

bkamins commented Aug 22, 2022

Thank you!

jonas-schulze added a commit to jonas-schulze/Compat.jl that referenced this pull request Oct 4, 2023
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.

3 participants