Closed jw3126 closed 6 years ago
I think that's an awesome idea!
For that, I'd suggest to add a @settable
macro for custom inner constructor like this:
@settable struct S{X, Y}
x::X
y::Y
S(x::X; y::Y=nothing) where {X, Y} = <function body>
end
expanding into
struct S{X, Y}
x::X
y::Y
S(x::X; y::Y=nothing) where {X, Y} = <function body>
S(x::X, y::Y=nothing) where {X, Y} = <function body> # positional-only version
end
Or @add_posonly
or @add_kwonly
.
But first let's see how the discussion on https://discourse.julialang.org/t/ann-setfield-jl-yet-another-package-for-working-with-immutables/9267/ goes
I also wonder if it can go into https://github.com/JuliaCollections
If we rely on a positional constructor and had the @setable
macro, would that cover all your usecases of Reconstructables.jl
conveniently? In order to apply @setable
, you need to own the type. So if there was a type that
1) has no positional constructor
2) has a keywords only constructor
3) is not owned by the user
Reconstructables.jl
would be more convenient. Does this situation arise in practice?
Yes, I think @settable
will cover all use-cases I have in mind (and more). I think DEProblem
subtypes in DiffEqBase.jl are good immutable struct examples. The package is not too complex but it is a basis of the large DifferentialEquations.jl ecosystem so it's a good specimen:
https://github.com/JuliaDiffEq/DiffEqBase.jl/tree/master/src/problems
I'm sure there are more but that's I can think of at the moment.
The three conditions you mentioned for keyword-only constructor is the only working option seems to be unreasonable. I don't think there is a way to solve set-ability problem for struct types you don't own without language support and without type piracy. And I agree the probability that a struct has positional-only constructor is higher.
The point in my comment on discourse is that, for users who want to have custom constructors, it's better to provide a macro to automate generation of positional-only. That's why I suggested @settable
.
The reason why I suggested @settable
over @add_posonly
(as in below) is that it can implement some sanity checks about field names and argument names are matching or not (in principle).
struct S{X, Y}
x::X
y::Y
@add_posonly S(x::X; y::Y=nothing) where {X, Y} = <function body>
end
Okay glad that @settable
is all we need. Before we implement it, we should check whether existing tools are sufficient / can be adapted.
DiffEq
using @with_kw
from Parameters.jl? It does leave the default constructor intact.I had a similar need, so I wrote a positional "constructor function": QuickTypes.construct
(unexported). Would that work for your purpose?
Well it would work, but it would also mean, that 1) This package needs to depend on QuickTypes 2) This package needs to special case types generated with QuickTypes
I am okay with 1), but I think 2) will cause headaches. Would it be an option that
@qstruct T(;a=2,b::String="") do
@assert length(b) == a
end
expands to something like
struct T
a::Any
b::String
function T(a::Any, b::String)
@assert length(b) == a
end
end
T(;a::Any=2, b::String="") = T(a,b)
Sorry, I would be uncomfortable keeping the default constructor at this point in time... One of the issues would be the special-case of certain constructors, like
struct FF
a
b
end
FF(a, b=2) = FF(a, b)
FF(1,3) # StackOverflow
In any case, if you're OK with depending on QuickTypes, then can't you just write?
QuickTypes.construct(typ, args...) = typ(args...)
Then your code calls QuickTypes.construct(typ, args...)
. It will work with quick- and non-quick-types.
Alternatively, you could provide unexported variants of @qstruct/@qmutable
so that SetField.@qstruct F(a; b=2)
expands into
QuickTypes.@qstruct F(a; b=2)
F(a, b) = QuickTypes.construct(F, a, b)
and tell your users to use
using SetField: @qstruct, @qmutable
if they are interested in QuickTypes macros?
Okay I understand the problem. I am curious, did you consider keeping the default constructor and came up with construct
as a solution to technical issues, or is construct
exactly what you desired in the first place? Anyway Setfield.@qstruct
is a good trick, thanks!
It's been too long, I don't really remember, which is why I'm wary of changing things :confused:. I think I wrote QuickTypes, then startied working with first-order logic, in which I had parametric objects like Add(Variable(:x), 2)
. Then performing variable substitution caused issues with parametric types (since the type of Add{Variable, Int}
changes). And that's how construct
, fieldsof
etc. came about.
Would it be possible to define the types in DiffEq using @with_kw from Parameters.jl? It does leave the default constructor intact.
Yea, I had the same question but it turned out it was not possible. This is because it needs to have custom type parameter which is nothing to do with field types. I don't think Parameters.jl or QuickTypes.jl can do it. In a very reduced manner it has to do something like:
struct Problem{isinplace,T}
some_field::T
Problem{isinplace}(some_field::T) where {isinplace,T} = new{isinplace,T}(some_field)
end
Notice that isinplace
type parameter has to be specified via Problem{isinplace}
. For real example, see:
https://github.com/JuliaDiffEq/DiffEqBase.jl/blob/master/src/problems/ode_problems.jl
I don't think we should rely on a central registry like QuickTypes.construct
. Package-independent calling convention like positional-only (or keyword-only) is much more reliable and flexible.
Re: stack overflow, if you write the inner constructor:
struct FF
a
b
FF(a, b=2) = new(a, b)
end
then it won't happen. For users who need keyword-based constructor, let them use @settable
macro:
@settable struct FF
a
b
FF(a; b=2) = new(a, b)
end
which expands to
struct FF
a
b
FF(a; b=2) = new(a, b)
FF(a, b) = new(a, b)
end
I think this approach is better since you can use any tools to do this AST transformation. For example, it works with Parameters.jl. On the other hand, if you need a central registry like QuickTypes.construct
, you have to use a tool that is compatible with QuickTypes.construct
.
Of course, QuickTypes.@qstruct
etc. can provide positional-only constructor if it is needed to be Setfield-compatible. Or, user can manually do:
Setfield.constructor_of(::MyType) = (args...) -> QuickTypes.construct(MyType, args...)
@cstjean Off topic, but @qstruct_fp
is great! That's exactly I need for auto-generating "differentiable lens".
I wouldn't mind adding a non-default keep_positional=true
option, either @qstruct Foo(...; _keep_positional=true)
, or with a p
suffix: @qstructp Foo(...)
Should be simple enough.
Yea, I had the same question but it turned out it was not possible. This is because it needs to have custom type parameter which is nothing to do with field types.
What do you mean? I solve my parametric type problems with QuickTypes.roottypeof
(which you can reproduce - it's not @qstruct
-specific):
@qstruct Foo{T}(; x::T=10)
f = Foo(; x=30)
QuickTypes.construct(QuickTypes.roottypeof(f), 2.3)
Does that help?
Here is an example from DiffEq
:
struct ODEProblem{uType,tType,isinplace,P,F,J,C,MM,PT} <:
AbstractODEProblem{uType,tType,isinplace}
f::F
u0::uType
tspan::Tuple{tType,tType}
p::P
jac_prototype::J
callback::C
mass_matrix::MM
problem_type::PT
@add_kwonly function ODEProblem{iip}(f,u0,tspan,p=nothing,
problem_type=StandardODEProblem();
jac_prototype = nothing,
callback=nothing,mass_matrix=I) where {iip}
if mass_matrix == I && typeof(f) <: Tuple
_mm = ((I for i in 1:length(f))...)
else
_mm = mass_matrix
end
new{typeof(u0),promote_type(map(typeof,tspan)...),
iip,typeof(p),typeof(f),typeof(jac_prototype),
typeof(callback),typeof(_mm),
typeof(problem_type)}(
f,u0,tspan,p,jac_prototype,callback,_mm,problem_type)
end
end
To me it seems that this actually has already two constructors, the one you see and one generated by add_kwonly
. I think allowing to @settable
a type with multiple inner constructors is problematic. We could arbitrarily choose the first constructor and ignore the others to generate the body of the positional constructor.
Another option is to add a @add_kwonly_and_positional
specifically for DiffEq
, which adds a keyword only and a positional constructor.
@cstjean No, @qstruct Foo{T}(; x::T=10)
is not what I meant. But if @qstruct Foo{another_type_parameter}(; x::T=10)
works then it's great (And I was wrong assuming that it doesn't work in QuickTypes.jl).
@jw3126 Yes it has two constructors. But @add_kwonly
is only for reconstructors (remake
function). If DiffEq were to use the keyword-only convention, I think @add_kwonly
can be removed.
I think allowing to
@settable
a type with multiple inner constructors is problematic.
I agree. I think we can implement some heuristics to choose the "best" inner constructor. A half-baked solution is in #18.
Okay, but at least for the deprecation phase of @add_kwonly
we have to support two constructors anyway. I would go with a version that is:
@add_kwonly
.@add_kwonly
Yes, that seems to be a good direction.
BTW, the reason why I bring up DiffEq is that it's a complex use-case of immutable struct that I know. I don't know if DiffEq is going to use Setfield.jl since it has re-constructor internally (note: it does not use Reconstructables.jl). If you want to set it up as one of the goals, I'd suggest to bring it up in https://github.com/JuliaDiffEq/DifferentialEquations.jl
One complication for using Setfield.jl in DiffEq is that definition of constructor_of
is going to require a major refactoring in DiffEqBase.jl. Since the structs in it have a custom type parameter (the iip
/isinplace
parameter) in the inner constructor, we need to customize constructor_of
. However there is no single abstruct type sharing the iip
type parameter with other structs. So, for Setfield.jl to work nicely, DiffEqBase.jl has to be refactored and introduce a new abstruct type. This abstruct type has to have iip
type parameter and all other structs with iip
type parameter must be subtypes of it.
Of course, we can add a custom @settable
-like macro which also define a custom constructor_of
. This does not require a refactoring but you need to write some other macro to extract iip
type parameter from the expression (which may not be so bad actually).
Okay, then I misunderstood this. I though it was one of the goals Reconstructables
to be used in DiffEq
. I am not very familiar with DiffEq
and can't judge for myself if it would benefit from Setfield.jl
.
DiffEq
easier?DiffEq
easier / more flexible?Sorry, I should have clarified that earlier. I wanted to use @add_kwonly
in other places without requiring DiffEqBase.jl and that's the primary reason why Reconstructables.jl exists. I also wondered whether using Reconstructables.jl from DiffEqBase.jl makes sense or not, but I thought that kind of suggestion can wait until I played with Reconstructables.jl enough to see the real benefits of it.
Would it make the code in
DiffEq
easier?
Well, you can replace the macros and function with a one line in REQUIRE. It's a seemingly trivial but actually big benefit, I think.
Would it make usage of
DiffEq
easier / more flexible?
There is not much facility for updating nested immutable struct at the moment in DiffEqBase. It works fine since most (but not all) of the structs are flat. But those structs keep user-defined "parameter" in a field. This parameter can be an immutable object as well. Updating values of this parameter is not easy. It'll be nice to have @set
in this case.
Any other benefits you see?
I wonder if the current status of Julia that has no easy way to update immutable struct, especially the nested ones, affect some design decisions of DiffEq (or any other libraries). So maybe start using Setfield.jl can facilitate more flexible design decision. OK. This is too abstract. I don't know other practical benefits at the moment. :)
So I shutdown Reconstructables.jl: https://github.com/tkf/Reconstructables.jl/commit/44d8206d45c465933155e221486d61ab9bd20344
Thanks for merging #17! I suppose we can close this issue now.
@tkf I think it would be nice to have only a single package for modifying immutable values. I propose to merge
Reconstructables
andSetfield
. Do you agree with this goal in principle? If so we can work out here, which features we want exactly.