-
Notifications
You must be signed in to change notification settings - Fork 152
add ConstructionBase interface #901
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
Conversation
0e96749
to
81427a8
Compare
The remaining test failures seem to be just ambiguities on 1.6 |
Thanks for the contribution. Dealing with immutable data structures is a big part of the core of this package. For example we have the I'm very glad people are thinking about these problems. If we can have a shared solution that has a chance of migrating towards We also use In generic code I might want to change the size or element type - so I'm a little curious how you handle this in practice with
Since Julia supports condition code loading - should we consider conditional methods either here or in ConstructorBase as the other option? |
There has been a bunch of thought on this in JuliaObjects and the various packages we've built separately. I guess having something widely supported that works is the way towards getting something similar in base. The idea with ConstructionBase.jl was centralizing the most fundamental methods to work with immutables and share the interface between all the packages that were experimenting with these things. But it's not really user-facing, I don't think most people know it exists until they get an error when My motivation is modifying immutable objects without knowing anything about their structure or specific methods they may have. e.g. packages like DynamicGrids.jl let users define a model however they want, but I still want to be able to e.g. strip the all the obj = Flatten.modify(x->Float32(x), obj, Float64) As long as Most of the other packages that use ConstructionBase - Accessor.jl, Setfield.jl, BangBang.jl are for updating known fields in immutable things. I've been going through and adding methods for base objects that don't construct, but feel free to make issues for any other Base methods that can't be updated and we can add them. To your specific comments, changing the size would be useful, and probably easy with an In terms of code loading, do you mean Requires.jl or just custom code? ConstructionBase.jl is a very small (100 loc) base package, so we would be wary to add anything to it like that. I've been thinking we could make it even smaller (e.g. ConstructionMethods.jl) which would just have the three empty methods to extend without the generated functions or anything else in ConstructionBase. Would that be better as a dependency here? |
I've just been reading over this over again as there was a lot to digest, and I missed some things you mentioned.
Rotations.jl returning a different object from You also mentioned |
Closing this in favor of ConsructionBaseExtras.jl adding these methods with Requires.jl as a stop-gap until we have first class conditional code loading. |
This PR adds an interface for ConstructionBase.jl.
This has been requested in #839. I realize there will be resistance to adding a dependency here, with good reason. But ConstrucitonBase.jl is a very small package that seems to make no difference to compile times here, and has some compelling use-cases.
StaticArrays are immutable. Modifying them requires calling their constructor. Packages like Flatten.jl, BangBand.jl, Setfield.jl, Accessors.jl facilitate modifying immutable objects. But objects with types that can't be derived from their fields, like
SArray
, need aconstructorof
method for this to work generically. Setfield.jl can already modifySArray
by setting array indices, but just replacing e.g. theSArray
tuple entirely in a generic way (that has no dep on StaticArrays) is not possible.To illustrate a little more, the main use cases I have for this PR are:
SArrray
) automatically with e.g. all Float64 swapped to Float32 For use on GPU, using Flatten.jl.Array
forCuArray
and similar while re-wrappingSizedArray
. The goal is that all arrays in an object can be swapped forCuArray
without needing an explicit Adapt.jl inteface, see e.g. Constructors for common base types? JuliaObjects/ConstructionBase.jl#34Static arrays are the most common objects that don't work with ConstuctionBase.jl, and the most friction for scaling this kind of functionality. Currently doing these things requires adding
constructorof
manually in a script.The other option is for this code to be moved into a glue package (maybe StaticArraysConstructors.jl at JuliaObjects). I haven't done that because a) it would be type pyracy, and should at least be blessed pyracy, and b) It would be a tiny package (10 lines) probably not worth the maintenance, c) that would require users to load and depend on it to get this functionality, instead of it just working, and d) packages that wanted to ensure that
constructorof
works in most case will need a StaticArrays.jl dependency.