-
Notifications
You must be signed in to change notification settings - Fork 147
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
Broadcast return type for MArray
s
#327
Comments
👍 we need to revisit this. The new changes seem like they've made a huge difference, but it's not all completely straightforward. We should test whether codegen is as good when an BTW, I can't foresee |
I agree, but what I don't think is necessary are the implicit conversions to so greatly favor |
I agree too. For example, when I add two MVectors, I expect the result to have the same type, especially if its an intermediate operation, and I need to mutate it further. |
Can this be revisited now that we're squarely in v1.0? We routinely run into this issue because all other mutable array types broadcast to mutable array types, usually retaining the type or choosing a sensible denser container. However, MArrays seem to be incompatible with any arithmetic since they always go to SArrays, so we get PRs to specifically add in type-casting only for MArrays to bring them back to MArrays. I think it's gone too far when downstream packages need to specifically revert the array type changes in order to have compatibility. While I agree users should just use SArrays in most cases, we're trying to set this up for their ease. |
It would be great if we could make this work better and I think this would probably be fairly non-breaking (provided performance is still good). I've added this to the list over at #532. |
It should be fine to adjust this here. Please keep in mind that this is a symptom of a deeper design problem. An So - we can happily adjust this here for the concrete type To fix this, we'd need a formal system of array traits (such as mutability - does |
Yes, we know that DiffEq cannot with every AbstractArray, but that's kind of a silly distinction. Technically to be compliant with AbstractArray you have just indexing and cannot assume broadcast, arithmetic, etc. exist, so we are using some subset already. And we put these in a parameterized type, so it requires an AbstractArray whose basic arithmetic is type-stable. Since we are doing math on it and we require efficiency, this doesn't seem to be a big cutoff for our use case. Of all of the input
I totally agree that this issue with MArray is just a symptom and not the real issue. But DiffEq is running into issues because we need all of this. I mean, the latest stuff I'm working on ( SciML/OrdinaryDiffEq.jl#549 ) probably looks absolutely bizarre since what is going on has absolutely no documented difference and is as of now just working off of known assumptions carried throughout the package ecosystem (i.e. JuliaLang/julia#21869 And that doesn't even cover a lot of the common issues we are now facing, for example the incompatibility of
With this experience we have far more than enough data to know what's required for these generic functions, yet no real documentation of how this is done. I would really enjoy if we could sit down and maybe gather @mbauman to really bang out some prototypes for trait-based array construction, generic array constructors, etc. The broadcast changes were great but were only the first step of what we need. |
Finally following through with it. We'll be developing it at https://github.com/JuliaDiffEq/ArrayInterface.jl and upstreaming to Base when we're happy with parts. |
Well, we've taken the plunge thanks to #536. |
With the coming changes in v1.0 allowing
MArray
s to stack-allocate:https://gist.github.com/ChrisRackauckas/7f3b1664c17ed8350888e217a8721e2f
there isn't a performance reason to prefer
SArray
over them. Thus, in light of the performance change, I think it may be wise to revisit the choice to makeu/t
always return anSArray
, for bothMArray
s andSArray
s. IMO it seems that simple arithmetic and broadcasting operations should keep mutability since that's a core property of the type, and without it generic algorithms can get messy:https://stackoverflow.com/questions/46985380/how-do-you-create-an-sarray-marray-with-changed-units
The text was updated successfully, but these errors were encountered: