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

Generalize BlockArray/BlockedArray to non-integer block lengths #405

Merged
merged 18 commits into from
Jun 17, 2024

Conversation

mtfishman
Copy link
Collaborator

The element types of BlockedUnitRange/BlockedOneTo were generalized to any Integer type in BlockArrays v1 (#255, #337, #395). However, the axes of BlockArray and BlockedArray are still hardcoded to have element type Int. This PR lifts that restriction.

Note that currently there is an issue printing very large BlockArray/BlockedArray:

julia> BlockedArray(1:2big(10)^10, [big(10)^10,big(10)^10])
2-blocked 20000000000-element BlockedVector{BigInt, UnitRange{BigInt}, Tuple{BlockedOneTo{BigInt, Vector{BigInt}}}}:
           1
           2
           3
           4
           5
           6
           7
           8
           9
          10
          11
          12
          13
          14
          15
          16
           
 19999999986
 19999999987
 19999999988
 19999999989
 19999999990
 19999999991
 19999999992
 19999999993
 19999999994
 19999999995
 19999999996
 19999999997
 19999999998
 19999999999
 20000000000

julia> BlockedArray(1:2big(10)^20, [big(10)^20,big(10)^20])
2-blocked 200000000000000000000-element BlockedVector{BigInt, UnitRange{BigInt}, Tuple{BlockedOneTo{BigInt, Vector{BigInt}}}}:
Error showing value of type BlockedVector{BigInt, UnitRange{BigInt}, Tuple{BlockedOneTo{BigInt, Vector{BigInt}}}}:
ERROR: InexactError: Int64(199999999999999999986)
Stacktrace:
  [1] Type
    @ ./gmp.jl:384 [inlined]
  [2] convert
    @ ./number.jl:7 [inlined]
  [3] to_index
    @ ./indices.jl:292 [inlined]
  [4] to_index
    @ ./indices.jl:277 [inlined]
  [5] _to_indices1
    @ ./indices.jl:359 [inlined]
  [6] to_indices
    @ ./indices.jl:354 [inlined]
  [7] to_indices
    @ ./indices.jl:350 [inlined]
  [8] isassigned(::BlockedVector{BigInt, UnitRange{BigInt}, Tuple{BlockedOneTo{BigInt, Vector{BigInt}}}}, ::BigInt, ::Int64)
    @ Base ./multidimensional.jl:1568
  [9] alignment(io::IOContext{…}, X::AbstractVecOrMat, rows::Vector{…}, cols::Vector{…}, cols_if_complete::Int64, cols_otherwise::Int64, sep::Int64, ncols::Int64)
    @ Base ./arrayshow.jl:68
 [10] _print_matrix(io::IOContext{…}, X::AbstractVecOrMat, pre::String, sep::String, post::String, hdots::String, vdots::String, ddots::String, hmod::Int64, vmod::Int64, rowsA::UnitRange{…}, colsA::UnitRange{…})
    @ Base ./arrayshow.jl:207
 [11] print_matrix(io::IOContext{…}, X::BlockedVector{…}, pre::String, sep::String, post::String, hdots::String, vdots::String, ddots::String, hmod::Int64, vmod::Int64)
    @ Base ./arrayshow.jl:171
 [12] print_matrix
    @ ./arrayshow.jl:171 [inlined]
 [13] print_array
    @ ./arrayshow.jl:358 [inlined]
 [14] show(io::IOContext{Base.TTY}, ::MIME{Symbol("text/plain")}, X::BlockedVector{BigInt, UnitRange{BigInt}, Tuple{BlockedOneTo{…}}})
    @ Base ./arrayshow.jl:399
 [15] (::OhMyREPL.var"#15#16"{REPL.REPLDisplay{REPL.LineEditREPL}, MIME{Symbol("text/plain")}, Base.RefValue{Any}})(io::IOContext{Base.TTY})
    @ OhMyREPL ~/.julia/packages/OhMyREPL/6UG7a/src/output_prompt_overwrite.jl:23
 [16] with_repl_linfo(f::Any, repl::REPL.LineEditREPL)
    @ REPL ~/.julia/juliaup/julia-1.10.4+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/REPL/src/REPL.jl:569
 [17] display
    @ ~/.julia/packages/OhMyREPL/6UG7a/src/output_prompt_overwrite.jl:6 [inlined]
 [18] display
    @ ~/.julia/juliaup/julia-1.10.4+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/REPL/src/REPL.jl:278 [inlined]
 [19] display(x::Any)
    @ Base.Multimedia ./multimedia.jl:340
 [20] print_response(errio::IO, response::Any, show_value::Bool, have_color::Bool, specialdisplay::Union{Nothing, AbstractDisplay})
    @ REPL ~/.julia/juliaup/julia-1.10.4+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/REPL/src/REPL.jl:0
 [21] (::REPL.var"#57#58"{REPL.LineEditREPL, Pair{Any, Bool}, Bool, Bool})(io::Any)
    @ REPL ~/.julia/juliaup/julia-1.10.4+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/REPL/src/REPL.jl:284
 [22] with_repl_linfo(f::Any, repl::REPL.LineEditREPL)
    @ REPL ~/.julia/juliaup/julia-1.10.4+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/REPL/src/REPL.jl:569
 [23] print_response(repl::REPL.AbstractREPL, response::Any, show_value::Bool, have_color::Bool)
    @ REPL ~/.julia/juliaup/julia-1.10.4+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/REPL/src/REPL.jl:282
 [24] (::REPL.var"#do_respond#80"{})(s::REPL.LineEdit.MIState, buf::Any, ok::Bool)
    @ REPL ~/.julia/juliaup/julia-1.10.4+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/REPL/src/REPL.jl:911
 [25] #invokelatest#2
    @ ./essentials.jl:892 [inlined]
 [26] invokelatest
    @ ./essentials.jl:889 [inlined]
 [27] run_interface(terminal::REPL.Terminals.TextTerminal, m::REPL.LineEdit.ModalInterface, s::REPL.LineEdit.MIState)
    @ REPL.LineEdit ~/.julia/juliaup/julia-1.10.4+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/REPL/src/LineEdit.jl:2656
 [28] run_frontend(repl::REPL.LineEditREPL, backend::REPL.REPLBackendRef)
    @ REPL ~/.julia/juliaup/julia-1.10.4+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/REPL/src/REPL.jl:1312
 [29] (::REPL.var"#62#68"{REPL.LineEditREPL, REPL.REPLBackendRef})()
    @ REPL ~/.julia/juliaup/julia-1.10.4+0.aarch64.apple.darwin14/share/julia/stdlib/v1.10/REPL/src/REPL.jl:386
Some type information was truncated. Use `show(err)` to see complete types.

This appears to be a Base Julia issue, see JuliaLang/julia#54786.

Copy link

codecov bot commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.25%. Comparing base (f762c3c) to head (d4b7a93).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #405      +/-   ##
==========================================
+ Coverage   94.99%   96.25%   +1.25%     
==========================================
  Files          16       18       +2     
  Lines        1498     1628     +130     
==========================================
+ Hits         1423     1567     +144     
+ Misses         75       61      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mtfishman
Copy link
Collaborator Author

Looks like there are some code coverage issues. Oddly those are not new lines of code but just ones that were modified by this PR, I guess they weren't covered before and modifying them triggered those coverage comments.

@dlfivefifty
Copy link
Member

It would be great if you can add tests even if its not your fault they weren't covered

@mtfishman
Copy link
Collaborator Author

Will do.

@mtfishman
Copy link
Collaborator Author

There's a few more code coverage issues to fix. Also I realized I need to rework the BlockIndexRange type a little bit to avoid abstract types. Working on those this afternoon.

@mtfishman
Copy link
Collaborator Author

mtfishman commented Jun 13, 2024

Ok, I redesigned BlockIndexRange to have all concrete/inferrable types and also fixed all of the code coverage issues.

I think there are a few more subtle places throughout the library that are hardcoded to Int but I would prefer to address those in future PRs since this PR has become pretty big already.

@mtfishman
Copy link
Collaborator Author

I noticed that there was a lot of code in blockaxis.jl that is still hardcoded to Int, I'm working on generalizing that now. After that this PR should be good to go from my end.

@mtfishman
Copy link
Collaborator Author

Ok, this is ready from my end.

@dlfivefifty dlfivefifty merged commit 9bad85a into JuliaArrays:master Jun 17, 2024
14 checks passed
@mtfishman mtfishman mentioned this pull request Jun 17, 2024
@mtfishman mtfishman deleted the generalize_blockarray branch June 17, 2024 15:14
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.

2 participants