Skip to content

PermutedDimsArray, and layout for dense 1st index#12

Merged
dlfivefifty merged 7 commits into
JuliaLinearAlgebra:masterfrom
mcabbott:permute
Apr 29, 2020
Merged

PermutedDimsArray, and layout for dense 1st index#12
dlfivefifty merged 7 commits into
JuliaLinearAlgebra:masterfrom
mcabbott:permute

Conversation

@mcabbott

@mcabbott mcabbott commented Apr 2, 2020

Copy link
Copy Markdown
Contributor

Closes #10

Not entirely sure whether SecondMajor will be needed in the end. (Nor what named things should have.)

@codecov

codecov Bot commented Apr 2, 2020

Copy link
Copy Markdown

Codecov Report

Merging #12 into master will increase coverage by 0.20%.
The diff coverage is 64.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #12      +/-   ##
==========================================
+ Coverage   61.14%   61.34%   +0.20%     
==========================================
  Files           8        8              
  Lines         700      727      +27     
==========================================
+ Hits          428      446      +18     
- Misses        272      281       +9     
Impacted Files Coverage Δ
src/ArrayLayouts.jl 42.25% <ø> (ø)
src/memorylayout.jl 68.50% <64.28%> (-0.33%) ⬇️

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 81f8ff8...84854d9. Read the comment docs.

Comment thread src/memorylayout.jl Outdated
Comment thread src/memorylayout.jl Outdated
"""
FirstMajor

const FirstUnion = Union{FirstMajor, AbstractColumnMajor}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this needs to be in ArrayLayouts.jl: I tend to avoid union types due to compile time type inference issues (though something this simple won't be impacted to that) and its only used in one place.

If there's a strong reason to keep it, it needs a more descriptive name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, I removed this for now.

The argument for it is that you would almost never want to dispatch on UnitStride{1} and exclude ColumnMajor, it just happens that there's no abstract type which fills this role.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, I'm tempted to remove IncreasingStrides and define

const ColumnMajor = UnitStride{1}

though perhaps some more thought is needed...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you mean ColumnMajor = Union{DenseColumnMajor, UnitStride{1}} as a replacement for AbstractColumnMajor?

One could also re-organise the type tree so that these two share a supertype. Does anything care about increasing strides, as opposed to just DenseColumnMajor or UnitStride{1}?

However I also need UnitStride{2}, and clearly the tree can't depend on D.

@dlfivefifty

Copy link
Copy Markdown
Member

Looks good! If you address the minor comments and get the code coverage tests passing I'll merge.

Comment thread src/memorylayout.jl Outdated
@mcabbott mcabbott marked this pull request as ready for review April 3, 2020 12:16
@dlfivefifty dlfivefifty merged commit 6c43de9 into JuliaLinearAlgebra:master Apr 29, 2020
@mcabbott mcabbott deleted the permute branch April 29, 2020 08:05
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.

PermutedDimsArray

2 participants