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

Type stability fix #230

Merged
merged 1 commit into from
Dec 31, 2021
Merged

Type stability fix #230

merged 1 commit into from
Dec 31, 2021

Conversation

chriselrod
Copy link
Collaborator

Tests still fail, but at least the inference tests no longer do:

to_indices: Test Failed at /Users/chriselrod/.julia/dev/ArrayInterface/test/indexing.jl:48
  Expression: #= /Users/chriselrod/.julia/dev/ArrayInterface/test/indexing.jl:48 =# @inferred(ArrayInterface.to_indices(a, inds)) == inds == (ArrayInterface.indices(a),)
   Evaluated: (Base.Slice(1:1:2),) == (Base.Slice(1:1:2),) == (Base.Slice(1:1:4),)
Stacktrace:
 [1] macro expansion
   @ ~/Documents/languages/julia/usr/share/julia/stdlib/v1.8/Test/src/Test.jl:479 [inlined]
 [2] macro expansion
   @ ~/.julia/dev/ArrayInterface/test/indexing.jl:48 [inlined]
 [3] macro expansion
   @ ~/Documents/languages/julia/usr/share/julia/stdlib/v1.8/Test/src/Test.jl:1375 [inlined]
 [4] top-level scope
   @ ~/.julia/dev/ArrayInterface/test/indexing.jl:33
to_indices: Test Failed at /Users/chriselrod/.julia/dev/ArrayInterface/test/indexing.jl:55
  Expression: #= /Users/chriselrod/.julia/dev/ArrayInterface/test/indexing.jl:55 =# @inferred(ArrayInterface.to_indices(a, (1, :))) == (1, Base.Slice(1:2))
   Evaluated: (1, Base.Slice(1:1:1)) == (1, Base.Slice(1:2))
Stacktrace:
 [1] macro expansion
   @ ~/Documents/languages/julia/usr/share/julia/stdlib/v1.8/Test/src/Test.jl:479 [inlined]
 [2] macro expansion
   @ ~/.julia/dev/ArrayInterface/test/indexing.jl:55 [inlined]
 [3] macro expansion
   @ ~/Documents/languages/julia/usr/share/julia/stdlib/v1.8/Test/src/Test.jl:1375 [inlined]
 [4] top-level scope
   @ ~/.julia/dev/ArrayInterface/test/indexing.jl:33
to_indices: Test Failed at /Users/chriselrod/.julia/dev/ArrayInterface/test/indexing.jl:57
  Expression: #= /Users/chriselrod/.julia/dev/ArrayInterface/test/indexing.jl:57 =# @inferred(ArrayInterface.to_indices(a, ([true, true], :))) == (Base.LogicalIndex(Bool[1, 1]), Base.Slice(1:2))
   Evaluated: ([1, 2], Base.Slice(1:1:1)) == ([1, 2], Base.Slice(1:2))
Stacktrace:
 [1] macro expansion
   @ ~/Documents/languages/julia/usr/share/julia/stdlib/v1.8/Test/src/Test.jl:479 [inlined]
 [2] macro expansion
   @ ~/.julia/dev/ArrayInterface/test/indexing.jl:57 [inlined]
 [3] macro expansion
   @ ~/Documents/languages/julia/usr/share/julia/stdlib/v1.8/Test/src/Test.jl:1375 [inlined]
 [4] top-level scope
   @ ~/.julia/dev/ArrayInterface/test/indexing.jl:33
Test Summary:     | Pass  Fail  Total  Time
to_indices        |   21     3     24  0.9s
  linear indexing |    4            4  0.0s
ERROR: LoadError: Some tests did not pass: 21 passed, 3 failed, 0 errored, 0 broken.
in expression starting at /Users/chriselrod/.julia/dev/ArrayInterface/test/indexing.jl:32
in expression starting at /Users/chriselrod/.julia/dev/ArrayInterface/test/runtests.jl:831

Comment on lines -448 to -454
function defines_strides(::Type{T}) where {T}
if parent_type(T) <: T
return false
else
return defines_strides(parent_type(T))
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ouch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should file a Julia issue on the branch no longer inferring?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this seems like a pretty rough case to not specialize.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks similar to JuliaLang/julia#43368.

@codecov
Copy link

codecov bot commented Dec 22, 2021

Codecov Report

Merging #230 (3c4581a) into master (ec8d2c7) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #230   +/-   ##
=======================================
  Coverage   88.06%   88.06%           
=======================================
  Files          11       11           
  Lines        1592     1592           
=======================================
  Hits         1402     1402           
  Misses        190      190           
Impacted Files Coverage Δ
src/ArrayInterface.jl 85.87% <100.00%> (ø)

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 ec8d2c7...3c4581a. Read the comment docs.

@Tokazama
Copy link
Member

@chriselrod, does this need a review or are we waiting on upstream changes?

@chriselrod
Copy link
Collaborator Author

chriselrod commented Dec 31, 2021

@chriselrod, does this need a review or are we waiting on upstream changes?

Needs a review. I don't think we should wait for JuliaLang/julia#43368 to get fixed.

@chriselrod chriselrod merged commit 8c2b84f into master Dec 31, 2021
@chriselrod chriselrod deleted the fixinference branch December 31, 2021 12:50
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.

4 participants