-
-
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
Add Memory
type
#51319
Add Memory
type
#51319
Conversation
base/genericmemory.jl
Outdated
## genericmemory.jl: Managed Memory | ||
|
||
""" | ||
GenericMemory{isatomic, T} <: AbstractVector{T} |
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.
Just a minor note that the name GenericMemory
doesn't really make it obvious that this subtypes AbstractArray
. Perhaps that's intentional (i.e. we don't want people to treat these too strongly as "arrays").
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 really think this shouldn't subtype AbstractVector
. I doubt you want to use a "named" GenericMemory
as input to all kinds of linear algebra functions etc. This should IMO be a very "basic" type with not many methods defined on it.
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.
The punning of "mathematical vector" vs. "generic store of a collection of objects" on AbstractVector
is indeed a bit unfortunate. This could be a good opportunity to disentangle the two - the question is just, what to do with existing uses of AbstractVector
? We may have to accept the punning, as GenericMemory
really does fulfill the minimal required 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.
the question is just, what to do with existing uses of AbstractVector?
Nothing? All I am saying is that I don't think the existing generic methods written for AbstractArray
will be very useful for GenericMemory
. Even though you can implement size
and getindex
, it doesn't mean you have to make it an AbstractArray
because you then opt into things that you might not want.
Many methods on AbstractArray
return an Array
unless you specialize them but it seems that is rarely something you want to happen if you have a GenericMemory
. So people will start overloading these methods to return a GenericMemory
etc and this just doesn't seem like something you want to deal with.
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.
That makes sense, yes. I'm on board with keeping this as its own thing, and having actual array semantics on different types on top of this.
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.
In fact, it is much more descriptive of a mathematical vector than Vector was, since Vector adds lots of features as "general collection of objects" which are not mathematically-oriented and do not exist for Memory.
You want to have a basic memory type that is free from even being thought about in the same way as a mathematical vector. If you want you can wrap something on top of Memory
that you consider an AbstractVector
but the core memory type should not be passable into ForwardDiff.gradient
and to .+
and a bunch of other stuff that doesn't make sense for it.
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.
the counterpoint is that given that it has a size, and getindex, you will get sensible results out of all of those.
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.
That's not really a counterpoint. A Char
implements those method but is not a subtype of AbstractArray
. The question is not what methods it implements, it is what this thing actually represents. And I think this should be free from any association with anything "mathy". ::Memory + ::Memory
doesn't really make sense at a conceptual level.
Just to bring up some questions that need to be answered:
- What should
::Memory .+ ::Memory
return? - What parts should
Vector
implement thatMemory
does not; is the only difference betweenVector
andMemory
thatVector
has size modifying functions? - Should
ForwardDiff.gradient
now returnMemory
(butForwardDiff.hessian
still has to return aMatrix
)? The argumentation above ("it is much more descriptive of a mathematical vector than Vector") seems to suggest it should. - Can you pass
Memory
to OpenBLAS? - Will almost everything that now takes an
Vector
start takingUnion{Vector, Memory}
? Should that apply to the ecosystem at large?
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.
Only to answer the last question, which may answer most of the others, they should be taking a DenseArray already, so no changes are needed for them. (Except that reminds me that this should really be a DenseVector, and not merely an AbstractVector. Usually I don't bother to express that level of granularity in array APIs, so it had slipped my mind.)
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.
which may answer most of the others
I don't really see how the other questions are answered from this.
Sorry for the bikeshedding, but given that we already have |
That's a good point, but I reason that |
Some questions that came up while reading the design document - documenting here so they're not lost to the triage hole:
|
|
0270f4e
to
72f67ce
Compare
I'm trying to help where I can with review and documentation here, but it's a lot to sift through. Do we still need to bike-shed the resize/grow/extend stuff, or is there something else more helpful you'd like more eyes on? |
We are slowly cranking through the todo list items listed in the commit comments (like making sure everything is documented and tested), and making sure all of the tests continue to pass. If people want to help, they are welcome to claim items via Slack for real-time collaboration on them |
base/Base.jl
Outdated
const DL_LOAD_PATH = String[] | ||
let os = ccall(:jl_get_UNAME, Any, ()) | ||
if os === :Darwin || os === :Apple | ||
if Base.DARWIN_FRAMEWORK |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
base/Base.jl
Outdated
const _included_files = Array{Tuple{Module,String},1}(Core.undef, 1) | ||
# start this big so that we don't have to resize before we have defined how to grow an array | ||
const _included_files = Array{Tuple{Module,String},1}(Core.undef, 50) | ||
_included_files.size = (1,) |
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.
Just a small thing — perhaps we should specialize getproperty
to make Array
look fully opaque from the REPL (and require get/set field for this kind of internals muckery). Given other languages, arr.size
is a really tempting footgun.
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.
yeah that's a good point. As nice as it made coding these, they should probably require a setfield
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.
Perhaps a name other than size
could serve an additional purpose, to further reduce "just access this"? Something like dims_tuple
?
Maybe not a good idea..
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.
What does setfield
has to do with REPL autocompletions?
And what's wrong with Matt's original suggestion again?
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.
what I mean is that if we overload getproperty
(and presumably setproperty
) you would need setfield
to modify the size. I was agreeing with the original suggestion.
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.
Isn't it enough to overload propertynames
which the REPL use for autocomplete?
3f2138d
to
80a67bc
Compare
b524983
to
e89d249
Compare
This was recently refactored in #51319 and while trying to adapt `libcxxwrap-julia` to the new changes I got: ``` ld: CMakeFiles/test_module.dir/test_module.cpp.o: in function `jl_datatype_layout': test_module.cpp:(.text+0x2c2): undefined reference to `jl_unwrap_unionall' ``` The function `jl_datatype_layout` is `STATIC_INLINE` in `julia.h`: https://github.com/JuliaLang/julia/blob/98542d748540e2390d6222282749c7dd534544da/src/julia.h#L1304-L1312 `jl_unwrap_unionall` is marked `JL_DLLEXPORT` here but it doesn't appear in the exports of `libjulia`.
Seems like a great improvement, but unfortunately it also broke the Julia C API. As a consequence CxxWrap (and probably a few other things) don't work anymore and need to be fixed. @benlorenz started to work on fixing it, see JuliaInterop/libcxxwrap-julia#137, but it's not yet working. To a degree, I understand why API breakage keeps happening, but it'd be nice if we could slowly over time start to stabilize some APIs and make some promises about keeping them around a bit longer -- APIs for accessing arrays seem like a prime candidate for that. Otherwise, certain packages which are not very actively maintained will just rot away over time, I am afraid. |
`cconvert` does not return an array anymore and cannot be used with `reinterpret`. Fix to use the underlying `transcode` function directly, which is also consistent with the `Cstring` version.
* Fix breakage due to JuliaLang/julia#51319 `cconvert` does not return an array anymore and cannot be used with `reinterpret`. Fix to use the underlying `transcode` function directly, which is also consistent with the `Cstring` version. * Fix doc test
…52248) This is used in an example here in the docs https://docs.julialang.org/en/v1.11-dev/manual/embedding/#Multidimensional-Arrays and needed to adapt to the removed `jl_alloc_array_2d` (#51319).
This is the implementation of https://hackmd.io/NnLXBeoyRymWgPtHYlW7-A?view#New-Builtin-functions
Fixes #24909