v1.0

[dlfivefifty]

1. Introduce BlockedOneTo
2. Redesign blocksizes
3. Rename PseudoBlockArray as BlockedArray
4. Various other fixes and changes

[codecov]

Codecov Report

Attention: Patch coverage is 94.78908% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 95.50%. Comparing base (ee08389) to head (70e6e02).
Report is 11 commits behind head on master.

Files	Patch %	Lines
src/blockedarray.jl	87.50%	14 Missing ⚠️
src/blockaxis.jl	97.05%	3 Missing ⚠️
src/blockbroadcast.jl	90.90%	2 Missing ⚠️
src/blockcholesky.jl	0.00%	1 Missing ⚠️
src/views.jl	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #255      +/-   ##
==========================================
+ Coverage   94.53%   95.50%   +0.97%     
==========================================
  Files          16       18       +2     
  Lines        1501     1625     +124     
==========================================
+ Hits         1419     1552     +133     
+ Misses         82       73       -9     

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

[dlfivefifty]

@jishnub I think the LazyArrays.jl extension should probably live in LazyArrays.jl.

Partly because a lot of LazyBandedMatrices.jl should be moved there. It'll be a lot of work to move that over...

[dlfivefifty]

@jishnub do you know what's going on with the CI?

[jishnub]

Do you mean why the checks are being skipped? I've used a plugin to skip duplicate checks if a commit has already been tested.

[dlfivefifty]

@jishnub I don't understand this change. Can you give an example where they are different?

[dlfivefifty]

This also will break a lot of smart block ranges eg:

julia> cumsum(Fill(2,10))
2:2:20

julia> accumulate(+,Fill(2,10))
10-element Vector{Int64}:
  2
  4
  6
  8
 10
 12
 14
 16
 18
 20

[dlfivefifty]

Ah ok I tried reverting the change and I see the issue.

I don't like the special cases below, instead we should specialise accumulate(+, ::AbstractFill).

[dlfivefifty]

@jishnub @mtfishman I have essentially all my downstream packages working. I think this is ready to merge and tag. Anything else you wanted merged in first?

[mtfishman]

The only potentially breaking change I would maybe want to see is changing the definition of blocksizes as discussed in #369. I would like to see some discussion of that, I'm basically convinced that the current definition doesn't make much sense and isn't very useful, and suggested a more useful definition in #369.

I'm also not entirely convinced of the current slicing behavior of BlockArrays/AbstractBlockedUnitRange when they are sliced with UnitRange, i.e. #347, but I see the argument from @jishnub that preserving the axes of the indices used in the slicing operation is more consistent with conventions in other parts of the Julia ecosystem (besides slicing with Colon) and the functionality I was hoping for can be added later with additional syntax like @blocked A[1:3, 1:3].

[mtfishman]

Not sure if #373 will also be considered for part of the v1 release?

[mtfishman]

In an ideal world we would have better names for the two concepts "tuple containing the number of blocks" (currently called blocksize) and "the size of specified block" (your proposed blocksizes). Along with the axes analogues. Unfortunately I can't think of better names. In the interest of not dragging things out (I'd like to get this tagged so the downstream changes can be merged) I would suggest we just implement your suggestion and leave any further renaming to v2.0.

Agreed, I think it will be confusing for new users that blocksize and blocksizes have such similar names but have quite different meanings, and it feels very natural to want something like blocksize(a::Array, b::Block) for getting the size of a block. I also can't think of an alternative that isn't worse in some way or another, or at least not clearly better. My guess is that a new user's first guess for trying to get the equivalent of blocksize would be to look for a function called nblocks but I see why that wasn't chosen.

A more incremental renaming that could be beneficial for disambiguating blocksize and blocksizes could be to rename blocksize to blockedsize (and similarly blocklength to blockedlength and blockaxes to blockedaxes), any thoughts on that? Modulo that potential change, it seems like a good time to tag v1.0.

[dlfivefifty]

would be to look for a function called nblocks but I see why that wasn't chosen

I think it actually used to be called nblocks but I renamed it since that naming convention didn't exist elsewhere in Julia. I wanted to mimic the length/size/axes naming convention as much as possible.

A more incremental renaming that could be beneficial for disambiguating blocksize and blocksizes could be to rename blocksize to blockedsize (and similarly blocklength to blockedlength and blockaxes to blockedaxes), any thoughts on that? Modulo that potential change, it seems like a good time to tag v1.0.

I'd be open to it but I'm not convinced that it is better, or if it's better that it's enough better to merit rewriting a bunch of things and delaying the release. I'm also trying to decide whether the rhyming with blockedrange is appropriate.

My instinct is to leave things as-is since its not too hard to understand and the docs point to to the other functions.

[mtfishman]

Since I learned about BlockedUnitRange I actually was thinking that it would be nice to lean into the term "blocked" even more throughout the package, for example PseudoBlockArray could be renamed BlockedArray (which I think is a good name since it makes it clearer that it is an array that has been blocked, i.e. indicates more clearly that it is a wrapper around an array that adds on a blocking structure), and then blockedsize, blockedlength, etc. would be consistent with that naming scheme. But I could see why you might be hesitant to make that change at this point, and there would be a clear upgrade path to that kind of naming scheme in v2 or v3 since it doesn't overlap with current names.

[dlfivefifty]

Actually I like BlockedArray a lot. PseudoBlockArray was never a very good name and BlockedArray does mirror BlockedUnitRange rather nicely. I would vote for making that change in v1.0.

@jishnub any opinions on this?

[dlfivefifty]

Also @KristofferC please let us know if you have any opinions on changes as creator of this package!

[jishnub]

Is the proposal here to rename PseudoBlockArray to BlockedArray, and leave BlockArray as is?

[mtfishman]

Is the proposal here to rename PseudoBlockArray to BlockedArray, and leave BlockArray as is?

That's what I had in mind.

[jishnub]

The change makes sense to me, although I wonder if the two names being so similar might lead to confusion?

[dlfivefifty]

Everything leads to confusion 🤷‍♂️ PseudoBlockArray definitely leads to confusion.

[jishnub]

Alright, I don't have issues with this change, so perhaps we should go ahead

[mtfishman]

I made #400 to track this issue. I'm happy to make a PR this week.

[jishnub]

We should probably have a NEWS.md in the repo where we list the major changes to make it easier to upgrade.

[dlfivefifty]

I don't necessarily think NEWS.md is a good idea. They add extra maintenance burden and inevitably become out-of-date, and Git commit history serves the same purpose.

[mtfishman]

What about a section in the README that lists the breaking changes of v1?

[dlfivefifty]

Sure. We can add an example of BlockedArray while we are at it

[mtfishman]

Other things to potentially mention:

• The definition of blocksizes(array::AbstractArray) is changed from blocklengths.(axes(array)) to an iterator of size blocksize(array) over the sizes of each block of array.
• BlockedUnitRange is now parametrized by the element type instead of hardcoded to Int.

[mtfishman]

Great to see this merged! Thanks for all of your work on this package @jishnub @dlfivefifty (and previous maintainers). We're planning to depend on this package quite a lot for our tensor network ecosystem, we currently have a pretty ad-hoc block sparse tensor type which we are rewriting from scratch using the BlockArrays package and it has really simplified our lives quite a bit having a well thought out block array interface to work from. It has made me wonder why distributed array types have been re-implementing their own block array-like interfaces instead of using the BlockArrays interface since it seems like a natural fit for that but I suppose that is a different story.

[dlfivefifty]

I don’t think it’s much of mystery: often when someone looks at a package designed by someone else they get put off if the design doesn’t match their expectations. This is made worse if the first attempt doesn’t work or if they encounter a bug. Then one decides they’re better of just designing their system from scratch that does match what they expect.

Unfortunately there’s not much that can be done other than make PRs to replace custom designs with ones based on this package…. but it’s not something I’m inclined to spend time worrying about.

In any case I’m really glad to hear this package is useful and hopefully the package will continue to improve as a result.