Skip to content

Incorporate blocks into axis, support offset blocks #95

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

Merged
merged 44 commits into from
Dec 23, 2019
Merged

Conversation

dlfivefifty
Copy link
Member

@dlfivefifty dlfivefifty commented Nov 28, 2019

This redesign when complete should resolve #26, #59, #91, and also support offset block ranges.

@codecov-io
Copy link

codecov-io commented Nov 28, 2019

Codecov Report

Merging #95 into master will decrease coverage by 0.25%.
The diff coverage is 79.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
- Coverage   80.69%   80.43%   -0.26%     
==========================================
  Files          12       10       -2     
  Lines         637      639       +2     
==========================================
  Hits          514      514              
- Misses        123      125       +2
Impacted Files Coverage Δ
src/BlockArrays.jl 100% <ø> (ø) ⬆️
src/blockarrayinterface.jl 100% <100%> (+14.28%) ⬆️
src/show.jl 96.55% <100%> (+0.12%) ⬆️
src/views.jl 57.77% <52.17%> (-19.31%) ⬇️
src/blockindices.jl 58.46% <61.01%> (+0.12%) ⬆️
src/abstractblockarray.jl 86.27% <86.66%> (-0.22%) ⬇️
src/pseudo_blockarray.jl 84.41% <90%> (-1.12%) ⬇️
src/blockbroadcast.jl 79.45% <92.85%> (+6.84%) ⬆️
src/blockarray.jl 89.05% <96.07%> (+0.91%) ⬆️
src/blockaxis.jl 98.41% <98.41%> (ø)
... 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 b4e9812...4521347. Read the comment docs.

@dlfivefifty dlfivefifty changed the title WIP Incorporate blocks into axis, support offset blocks Incorporate blocks into axis, support offset blocks Dec 2, 2019
@dlfivefifty
Copy link
Member Author

dlfivefifty commented Dec 2, 2019

The tests are passing (at least locally) so it would be nice to get a review. @tkf would you be able to do this? I still need to update the docs before this gets merges.

This PR changes it so block sizes are incorporated into directly into the axis. This means instead of a single type BlockSize with a tuple. Another change is that we call cumsum, replacing the by-hand construction in cumul_sizes. The benefit of this is the package takes advantage of types with fast cumsum: for example, we get essentially for free support for fixed size blocks via FillArrays.jl since cumsum(Fill(3,10)) == 3:3:30.

Another benefit is that we get free support for offset block indexing using OffsetArrays.jl. This was mostly done for "fun" (or rather, as an excuse to merge this PR), though there are cases where this will be useful.

In my basic performance tests there is negligible performance change; e.g., A[1,1] for a BlockArray takes 32ns now instead of 29ns as before.

@KristofferC If you are still using this package, do you have any opinions?

EDIT: I think I ran the tests in the wrong window so still need to make some fixes to get broadcasting working.

@dlfivefifty dlfivefifty changed the title Incorporate blocks into axis, support offset blocks WIP Incorporate blocks into axis, support offset blocks Dec 2, 2019
@tkf
Copy link
Member

tkf commented Dec 3, 2019

Sorry, I can't review this ATM, at least until this weekend.

@dlfivefifty
Copy link
Member Author

No worries, no rush on my end.

@dlfivefifty
Copy link
Member Author

@tkf The tests are now passing if you find time to have a look.

@dlfivefifty dlfivefifty changed the title WIP Incorporate blocks into axis, support offset blocks Incorporate blocks into axis, support offset blocks Dec 18, 2019
@tkf
Copy link
Member

tkf commented Dec 19, 2019

Still don't have time to review this in detail (BTW, please don't wait for me if this is a blocker), but...

The benefit of this is the package takes advantage of types with fast cumsum: for example, we get essentially for free support for fixed size blocks via FillArrays.jl since cumsum(Fill(3,10)) == 3:3:30.

This is really cool! I suppose another advantage is that it works for statically-encoded blocksize? I see that something like BlockArray(rand(4, 4), SVector(2,2), SVector(1,1,2)) works. (ATM, cumsum(SVector(2,2)) returns a MArray, but it's easy to fix.) Not sure if this matters performance-wise but nice to be able to do it anyway.

@tkf
Copy link
Member

tkf commented Dec 20, 2019

I looked at broadcasted code and it LGTM.

@dlfivefifty dlfivefifty merged commit 3addf8f into master Dec 23, 2019
@dlfivefifty dlfivefifty deleted the dl/blockaxis branch December 23, 2019 18:35
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.

Fast path for block sizes where all blocks have the same size
3 participants