-
-
Notifications
You must be signed in to change notification settings - Fork 63
Adds an abstract type to NamedArrayPartition #447
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
base: arraypart_zero
Are you sure you want to change the base?
Adds an abstract type to NamedArrayPartition #447
Conversation
In order to resolve SciML#443 where I would like to be able to subtype from NamedArrayPartition. This is a large restructuring of NamedArrayPartition but all tests within RecursiveArrayTools do pass. I'm not sure of other tests that I would need to run to ensure further compatibility. This maintains all current functionality but allows a user to create a new subtype of AbstractNamedArrayPartition as long as it maintains the same structure
src/named_array_partition.jl
Outdated
@@ -1,3 +1,5 @@ | |||
abstract type AbstractNamedArrayPartition{T, A, NT} <: AbstractVector{T} end |
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 this should have an AbstractArrayPartition, and that needs a docstring to describe its interface?
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.
Or is ArrayPartition
an AbstractNamedArrayPartition
with x
being its only names?
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 can add a higher level called AbstractArrayPartition{T} if you want and apply the same level of abstraction to the ArrayPartition interface as well (either or both) such that the same subtyping can be performed there also. I will add to the documentation, a docstring about how you can subtype from the AbstractNamedArrayPartition
|
||
function Base.similar(A::NamedArrayPartition) | ||
NamedArrayPartition( | ||
# With new type structure this function does the same as Base.similar(x::AbstractNamedArrayPartition{T, S, NT}) where {T, S, NT} |
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.
Why is this one commented?
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.
Because I couldn't manage in either the old or the new method to call the less specific function as providing an NamedArrayPartition will have the type structure X{N, A, NT} and therefore a later version would have instead been called. In the original code the two functions are:
function Base.similar(A::NamedArrayPartition)
NamedArrayPartition(
similar(getfield(A, :array_partition)), getfield(A, :names_to_indices))
end
function Base.similar(x::NamedArrayPartition{T, S, NT}) where {T, S, NT}
NamedArrayPartition{T, S, NT}(
similar(ArrayPartition(x)), getfield(x, :names_to_indices))
end
But in all cases I tested the second always overwrites the first as any NamedArrayPartition has the structure of the second
After @ChrisRackauckas comments I have documented how to subtype from AbstractNamedArrayPartition as well as added an AbstractArrayPartition type which both AbstractNamedArrayPartition and ArrayPartition subtype from
It's still missing what is meant by the interface. See for example https://github.com/SciML/RecursiveArrayTools.jl/blob/master/src/RecursiveArrayTools.jl#L16-L110. The questions that have to be answered are:
Then we need to make sure the abstract library implementations actually match this. For example, for
|
In order to resolve #443 where I would like to be able to subtype from NamedArrayPartition. This is a large restructuring of NamedArrayPartition but all tests within RecursiveArrayTools do pass. I'm not sure of other tests that I would need to run to ensure further compatibility. This maintains all current functionality but allows a user to create a new subtype of AbstractNamedArrayPartition as long as it maintains the same structure. I am pulling into this branch as this is where my previous fix to the ArrayInterface.zeromatrix lives.
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context