Skip to content
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

Bug in functor(::Type{NamedTuple}, Any) #38

Open
mcabbott opened this issue Feb 7, 2022 · 5 comments
Open

Bug in functor(::Type{NamedTuple}, Any) #38

mcabbott opened this issue Feb 7, 2022 · 5 comments

Comments

@mcabbott
Copy link
Member

mcabbott commented Feb 7, 2022

I think this is a bug -- functor(typeof(x), y) should always use x's field names on y:

julia> struct Foo; x; y; end

julia> @functor Foo

julia> struct AnotherFoo; x; y; end

julia> x = Foo([1, 2], 3);

julia> y = AnotherFoo([4, 5], 6);

julia> z = (x = [7, 8], y = 9);

julia> functor(x)
((x = [1, 2], y = 3), var"#31#32"())

julia> functor(typeof(x), y)
((x = [4, 5], y = 6), var"#31#32"())

julia> functor(typeof(z), y)  # this is wrong?
(AnotherFoo([4, 5], 6), Functors.var"#5#6"())
@ToucheSir
Copy link
Member

ToucheSir commented Feb 7, 2022

https://github.com/FluxML/Functors.jl/blob/v0.2.7/src/functor.jl#L5. Suffice it to say I don't think this case was considered when the base functor definitions were written...

@ToucheSir
Copy link
Member

(the Tuple and AbstractArray defs are likewise "fun")

@mcabbott
Copy link
Member Author

mcabbott commented Feb 7, 2022

I didn't think about those. You are suggesting that

julia> t = ([10,11], 12);

julia> functor(typeof(t), z)
((x = [7, 8], y = 9), Functors.var"#3#4"())

should return a tuple? No struct can match propertynames(t) I believe, but z[1] == z[:x] != getproperty(z, 1)... where's the line between using getproperty and using getindex?

@ToucheSir
Copy link
Member

I'm not sure. This is one of those areas where the original codebase played fast and loose with what behaviour is "in spec", so it's not clear to me what the intent was. I do think that it shouldn't pass x through like it does now if the NamedTuple case is changed.

@mcabbott
Copy link
Member Author

mcabbott commented Feb 7, 2022

One attempt is:

functor(::Type{<:Tuple{Vararg{Any,N}}}, x) where N = NTuple{N}(x), y -> NTuple{N}(y)
functor(::Type{<:NamedTuple{L}}, x) where L = NamedTuple{L}(map(s -> getproperty(x, s), L)), y -> NamedTuple{L}(map(s -> getproperty(y, s), L))

I'm not sure the y -> ... half is necessary at all. Nor whether the Tuple case is desirable.

The NTuple case showed up in tests here: https://github.com/FluxML/Functors.jl/pull/37/files/4adbcf1614af31478bb56665c1fd8f66353e0b35..7ab2efdbc0e2b030397b6cd5e8a0ec6071fbcef1#diff-ff6ed95b5e91003416c0e6ea6e89f3cd7672e0b34da92293fa8db6632622c8ebR118

mcabbott added a commit to mcabbott/Functors.jl that referenced this issue Feb 7, 2022
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

No branches or pull requests

2 participants