Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

rafaqz
Copy link
Contributor

@rafaqz rafaqz commented Apr 23, 2021

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 a constructorof method for this to work generically. Setfield.jl can already modify SArray by setting array indices, but just replacing e.g. the SArray 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:

  • reconstructing large, complex objects (that often contain SArrray) automatically with e.g. all Float64 swapped to Float32 For use on GPU, using Flatten.jl.
  • ModelParamaters.jl working with static arrays of parameters - parameter wrappers can be placed anywhere in a model, and used for control or optimisation, and stripped out later. Currently they cant go in a static array because you can't remove them later.
  • Swapping Array for CuArray and similar while re-wrapping SizedArray. The goal is that all arrays in an object can be swapped for CuArray without needing an explicit Adapt.jl inteface, see e.g. Constructors for common base types? JuliaObjects/ConstructionBase.jl#34

Static 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.

@rafaqz rafaqz force-pushed the constructorof branch 2 times, most recently from 0e96749 to 81427a8 Compare April 24, 2021 05:27
@rafaqz
Copy link
Contributor Author

rafaqz commented Apr 24, 2021

The remaining test failures seem to be just ambiguities on 1.6

@andyferris
Copy link
Member

Thanks for the contribution.

Dealing with immutable data structures is a big part of the core of this package. For example we have the similar_type function (instead of similar) which returns a constructor and lets you chose the dimensions and element type. The constuctor must accept a Tuple. For generic programming with these "constructor factories" you often want to set some other traits of the container: like "I want to use setindex! and push! or not (similar implies mutability, for example, there's a copy and Base.copymutable (which might or might not be resizable), empty and emptymutable (which I presume implies resizable)). The collect function doesn't let you set any of these and there's no other "value constructor" for AbstractArray in Base

I'm very glad people are thinking about these problems. If we can have a shared solution that has a chance of migrating towards Base that would be fantastic. I keep thinking of how we should do this in Base with existing containers like Array, Dict, Set, etc, with at least immutable/mutable/resizable traits, but then there's all the "layout" traits like sparse.

We also use similar and similar_type on Rotations.jl arrays but they don't return a constructor for the same type. Do you have any idea of how they would support constructorof?

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 constructorof?

The other option is for this code to be moved into a glue package (maybe StaticArraysConstructors.jl at JuliaObjects).

Since Julia supports condition code loading - should we consider conditional methods either here or in ConstructorBase as the other option?

@rafaqz
Copy link
Contributor Author

rafaqz commented Apr 25, 2021

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 constructorof doesn't work.

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 Floa64 for Float32, or get model parameters to generate user interfaces automatically, wherever the parameters happen to be embedded in the object. For float replacement you can just do:

obj = Flatten.modify(x->Float32(x), obj, Float64)

As long as constructorof works generically on obj and its components, or is defined specifically for the types where it doesn't.

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 SVector to use the length of the tuple? I can add that to this PR. Maybe not with SMatrix generically? SizedArray could also be updated based on the array size instead of staying in the type, although you lose type stability for when you don't need to change the size.

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?

@rafaqz
Copy link
Contributor Author

rafaqz commented Apr 30, 2021

I've just been reading over this over again as there was a lot to digest, and I missed some things you mentioned.

similar_type does seem like a very similar concept to constructorof - except constructorof doesn't specify that it should actually be a type that is returned - it's just something that will construct the object, when given the output of ConstructionBase.getproperties as arguments. These are usually just all of the objects fields.

constructorof usually leaves the field types free to change, as we can get them from the arguments. It seems similar_type allows for a different type T in its 2 argument form.

Rotations.jl returning a different object from constructorof should be fine too, I don't think there is any restriction on the type of the returned object.

You also mentioned Dict and Set etc. These probably should have constructorof methods defined in ConstructionBase.jl, which would allow setting their fields manually with e.g. @set from Setfield.jl.

@rafaqz
Copy link
Contributor Author

rafaqz commented May 9, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants