-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC/WIP: Support array type as input for functions returning AbstractArray instance #16740
Closed
Closed
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
f1dda6a
Add check_array_size function
martinholters f79a270
New function LinAlg.checksquaredims
martinholters b6a71ba
Add Type(dims) constructors for BitArray and Diagonal
martinholters 4c7bc3f
Add Type(dims) constructors for SparseMatrixCSC and SparseVector
martinholters d68e857
Add Type(dims) constructors for Bidiagonal, Tridiagonal, and
martinholters 5c094ad
New function fixate_eltype
martinholters 0161c56
Add generic fill method accepting return type as argument
martinholters b0d7d70
Generic zeros, ones, and eye, taking the desired array type as first …
martinholters 7a0f34a
Rewrite spzeros and speye in terms of zeros and eye, respectively
martinholters f7a27dc
Generic typed_vcat, typed_hcat, typed_hvcat, and cat_t, taking the
martinholters d2b15db
Provide specialized typed_*cat(::Type{SparseMatrixCSC}, ...)
martinholters e7e9bb1
Provide specialized typed_[hv]cat(::Type{SparseVector}, ...)
martinholters File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the idea. Maybe we can do it even more general? E.g.
so it also gives
set_type(Diagonal{Int}, Float64) === Diagonal{Float64})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deliberately wanted
fixate_eltype(Diagonal{Int}, Float64) === Diagonal{Int}
to be able to specify a default element type. E.g.zeros(Diagonal, 2, 2)
should give aDiagonal{Float64}
, butzeros(Diagonal{Int}, 2, 2)
should give aDiagonal{Int}
. Maybe a third, boolean argument could be used to choose between "set always" and "set only if unset".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I had in mind another use case, using something like this combined with something like
deepcopy_internal
to write a genericsimilar(::AbstractArray, ::Type)
. Handling the dimensions in a generic fashion might not be possible and we would still need specialization (as you have here).With this in mind and what is needed here, a third boolean argument would seem like the way to cover both scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is fundamentally invalid.
T.name.primary
isn't a meaningful type and its type parameters don't have any fixed relation to the type parameters ofAbtractArray
. If you can't express a type relationship via dispatch, that is highly indicative that the relation you want is not valid.I think you want to call the
similar
function, whose definition is similar to what it appears the function above was attempting to do.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it could be something like
This would give
set_eltype(Diagonal, Float64) === Diagonal{Float64}
,set_eltype(Diagonal{Int}, Float64) === Diagonal{Int}
,set_eltype(Diagonal{Int}, Float64, true) === Diagonal{Float64}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've often wished that
similar(::Type, ...)
returned a type rather than an instance, and thatAbstractArray
subtypes implemented only it. Then a generic fallback would automatically return instances when passed instances. Wouldn't that be all we need here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing these insights, @vtjnash, it certainly is a valuable contribution to the manual. But I still don't see why the
fixate_eltype
from this PR would be invalid. As said, it seems to work as I want it:There is one corner case I could find:
I'm not sure what would be desired in this case, though. Maybe throwing instead of returning a type with still unspecified element type would be better. Anyway, this kind of type does not look exactly useful, so I'm not worried too much.
That sounds indeed worrying for the proposed
fixate_eltype
. However,T.name.primary
is used in a couple of places around base, so there has to be something of value in it. Also, the relationship of the type parameters between, say,Array
andAbstractArray
has be to recorded somewhere. Why shouldn't we be able to exploit it here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vtjnash Comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the conclusion is that this should be implemented with dispatch cf. the new paragraphs in the documentation and that
T.name.primary
should generally be avoided. The places in Base whereT.name.primary
are few and special.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OTOH https://github.com/JuliaLang/julia/blob/7f230adcf13d7a2bcb76e55327f211b7b4f25046/doc/devdocs/types.rst#typenames reads as though the use of
T.name.primary
here is perfectly valid. (Maybe the relationship of theTypeVar
s is a real problem?) It feels a bit unsatisfying to require a method to be implemented for every array type that should support e.g.zeros(MyArrayType, (5, 3))
when there is this generic alternative available.