Skip to content

Conversation

@Tokazama
Copy link
Member

@Tokazama Tokazama commented Jun 5, 2022

Using the heuristic parent_type(T) <: T sometimes gives incorrect results and may hide holes in the interface (as I found when making these changes).

Some problems I found:

  • is_increasing(stride_rank(A)) didn't account for A having zero dimensions
  • is_dense(A) didn't know how to handle when dense_dims(A) returned nothing.
  • contiguous_axis(::Type{<:ReshapedArray}) would sometimes return nothing (meaning the contiguous axis is unknown) instead of StaticInt(-1) when we actually knew there couldn't be a contiguous axis because it didn't exist in the parent array.
  • (known)_dimnames(A) was often returning incorrect results for ReinterpretArray and ReshapedArray.

@codecov
Copy link

codecov bot commented Jun 5, 2022

Codecov Report

Merging #296 (fe9307b) into master (a61ed56) will decrease coverage by 1.05%.
The diff coverage is 73.26%.

❗ Current head fe9307b differs from pull request most recent head f9410a1. Consider uploading reports for the commit f9410a1 to get more accurate results

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
- Coverage   91.07%   90.02%   -1.06%     
==========================================
  Files           9        9              
  Lines        1345     1393      +48     
==========================================
+ Hits         1225     1254      +29     
- Misses        120      139      +19     
Impacted Files Coverage Δ
src/axes.jl 92.56% <50.00%> (+0.25%) ⬆️
src/indexing.jl 87.86% <50.00%> (+0.48%) ⬆️
src/dimensions.jl 85.55% <56.25%> (-11.61%) ⬇️
src/stridelayout.jl 90.24% <93.10%> (+0.21%) ⬆️
src/ArrayInterface.jl 85.95% <100.00%> (-0.12%) ⬇️
src/size.jl 92.13% <100.00%> (+1.77%) ⬆️
... and 2 more

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 a61ed56...f9410a1. Read the comment docs.

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