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

Update BlockBandedMatrix iteration for new release #84

Merged
merged 9 commits into from Dec 28, 2019

Conversation

ChrisRackauckas
Copy link
Member

@dlfivefifty what's the right translation for BlockArrays.axes(BlockBandedMatrices.cumulsizes(Jac,1))?

@dlfivefifty
Copy link

I think just blockaxes(Jac,1). Though note that returns a Block - valued range

Copy link

@dlfivefifty dlfivefifty left a comment

Choose a reason for hiding this comment

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

Some comments, looks like more changes meeded

cs = BlockArrays.blocklasts(BlockArrays.axes(Jac,2))
b = BlockBandedMatrices.BlockArray(vfx,rs)
c = BlockBandedMatrices.BlockArray(colorvec,cs)
@inbounds for J=BlockBandedMatrices.Block.(1:BlockBandedMatrices.nblocks(Jac,2))

Choose a reason for hiding this comment

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

Should be J= blockaxes(Jac,2)

cs = BlockArrays.axes((BlockBandedMatrices.cumulsizes(Jac,2),))
b = BlockBandedMatrices.BlockArray(vfx,rs)
c = BlockBandedMatrices.BlockArray(colorvec,cs)
@inbounds for J=BlockBandedMatrices.Block.(1:BlockBandedMatrices.nblocks(Jac,2))

Choose a reason for hiding this comment

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

J = blockaxes(Jac,2)

@ChrisRackauckas
Copy link
Member Author

I think just blockaxes(Jac,1). Though note that returns a Block - valued range

Yeah, the fact that it's a Block-valued range seems to be an issue. Do I just need to turn those from Block to Int?

@dlfivefifty
Copy link

Int(::Block) turns a block into an Int

@ChrisRackauckas
Copy link
Member Author

Looks like whatever BlockBandedMatrices.cumulsizes was doing was needed, since it needs to be dome some kind of summation? I tried just using cumsum(Int.(cs)) but that doesn't seem to be it.

@dlfivefifty
Copy link

blocklasts does the same thing as the previous cumulsizes: it gives the last index in each block

@ChrisRackauckas
Copy link
Member Author

But it seems blocklasts can't act on what comes out of blockaxis?

@dlfivefifty
Copy link

Right, you need to call something like blocklasts(axes(A,1))

@ChrisRackauckas
Copy link
Member Author

DimensionMismatch("block size for dimension 1: [100, 200, 300, 400, 500, 600, 700, 800, 900, 1000, 1100, 1200, 1300, 1400, 1500, 1600, 1700, 1800, 1900, 2000, 2100, 2200, 2300, 2400, 2500, 2600, 2700, 2800, 2900, 3000, 3100, 3200, 3300, 3400, 3500, 3600, 3700, 3800, 3900, 4000, 4100, 4200, 4300, 4400, 4500, 4600, 4700, 4800, 4900, 5000, 5100, 5200, 5300, 5400, 5500, 5600, 5700, 5800, 5900, 6000, 6100, 6200, 6300, 6400, 6500, 6600, 
6700, 6800, 6900, 7000, 7100, 7200, 7300, 7400, 7500, 7600, 7700, 7800, 7900, 8000, 8100, 8200, 8300, 8400, 8500, 8600, 8700, 8800, 8900, 9000, 9100, 9200, 9300, 9400, 9500, 9600, 9700, 9800, 9900, 10000] does not sum to the array size: 10000")

It needs the opposite of cumsum or something?

src/iteration_utils.jl Outdated Show resolved Hide resolved
@ChrisRackauckas
Copy link
Member Author

This is likely the last one. I can't figure out what's supposed to replace the BlockBandedMatrices.blocksize(Jac,(blockcolrange[1].n[1],J.n[1])) call. From the API docs it looks like there's only BlockBandedMatrices.blocksize(Jac) dispatches now? I tried to see if I could find out where it went in JuliaArrays/BlockArrays.jl@3addf8f but couldn't find it.

@dlfivefifty
Copy link

I think:

length.(getindex.(axes(Jac), (blockcolrange[1], J)))

I could add a convenience function for this, but to be honest the old names in BlockArrays.jl we’re always confusing to me, so my preference is to be explicit instead of adding more to the interface.

@coveralls
Copy link

coveralls commented Dec 28, 2019

Coverage Status

Coverage decreased (-0.05%) to 73.185% when pulling 6132c12 on bbm_iteration into 2fa19f4 on master.

@codecov
Copy link

codecov bot commented Dec 28, 2019

Codecov Report

Merging #84 into master will decrease coverage by 0.88%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
- Coverage   74.07%   73.18%   -0.89%     
==========================================
  Files           8        8              
  Lines         513      496      -17     
==========================================
- Hits          380      363      -17     
  Misses        133      133
Impacted Files Coverage Δ
src/iteration_utils.jl 96% <100%> (-0.08%) ⬇️
src/gradients.jl 48.46% <0%> (-2.13%) ⬇️
src/derivatives.jl 69.56% <0%> (-1.27%) ⬇️
src/jacobians.jl 89.54% <0%> (-0.46%) ⬇️

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 2fa19f4...6132c12. Read the comment docs.

@ChrisRackauckas ChrisRackauckas changed the title [WIP] update BlockBandedMatrix iteration for new release Update BlockBandedMatrix iteration for new release Dec 28, 2019
@ChrisRackauckas ChrisRackauckas merged commit 03705af into master Dec 28, 2019
@ChrisRackauckas ChrisRackauckas deleted the bbm_iteration branch December 28, 2019 17:27
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.

None yet

3 participants