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

fmapstructure #14

Merged
merged 4 commits into from
Jul 9, 2021
Merged

fmapstructure #14

merged 4 commits into from
Jul 9, 2021

Conversation

ToucheSir
Copy link
Member

@ToucheSir ToucheSir commented Jul 1, 2021

Example usage:

m = Chain(Dense(10, 5), Dense(5, 2))

functorize(identity, m) |> typeof
# Tuple{NamedTuple{(:weight, :bias, :σ), Tuple{Matrix{Float32}, Vector{Float32}, typeof(identity)}}, NamedTuple{(:weight, :bias, :σ), Tuple{Matrix{Float32}, Vector{Float32}, typeof(identity)}}}

# Similar output to what you get from explicit gradients
functorize(x -> x isa AbstractArray ? x : nothing, m) |> typeof
# Tuple{NamedTuple{(:weight, :bias, :σ), Tuple{Matrix{Float32}, Vector{Float32}, Nothing}}, NamedTuple{(:weight, :bias, :σ), Tuple{Matrix{Float32}, Vector{Float32}, Nothing}}}

Note how the implementation is almost exactly the same as fmap, just without the re(construction). Ideally this functionality could be folded into fmap itself, but I'm not sure what the most elegant way to conditionally control reconstruct is.

cc @DhairyaLGandhi @darsnack

@DhairyaLGandhi
Copy link
Member

Nice, this is basically just traversing the tree, right? We could technically replace the construction with an identity. maybe an indirection with fmap1? I wish we have better names for these functions.

Also, we would miss out on non-functor-able objects in this view which is a shame.

@ToucheSir
Copy link
Member Author

Pretty much, I think we'd some kind of kwarg and an additional conditional for fmap, because re must be derived at runtime from x itself. Then there's the question of whether to pass that kwarg onto fmap1 and have it handle the conditional, or to make another function that works like fmap1 but ignores re.

Non-functorable objects are handled exactly the same way as they are in fmap, fcollect etc. Which is to say they're considered leaf nodes and retained in the tree as-is (assuming one uses exclude=isleaf).

@darsnack
Copy link
Member

darsnack commented Jul 1, 2021

Is fmap1 exported? We could just change the name and 0% of users will notice. Could be called applyfandre? Or maybe something better than that.

I don't think complicating fmap is the way to go. Functors.jl has essentially three functions right now. I would be in favor of just adding a new function like factorize instead of more kwargs to fmap.

@darsnack
Copy link
Member

darsnack commented Jul 1, 2021

If anything I feel like f here serves a similar role to exclude in fcollect. Would it make more sense to make fcollect maintain structure? You could easily flatten the nested tuples after the fact.

@ToucheSir
Copy link
Member Author

ToucheSir commented Jul 1, 2021

I feel like f here serves a similar role to exclude in fcollect

Not really, exclude signals when to stop recursing while f is a transformation function. Hence fmap is really a structure preserving map. The reason why fmap and functorize have so much duplication is because they have different definitions of "structure preserving":

  • fmap (currently) always maintains the original struct type. i.e. fmap(identity, m) == m.
  • functorize maintains only the structure and values of the original struct.

Come to think of it, we could solve this just by having fmap1 as a parameter of fmap. Then the default function would be fmap1, and to replicate functorize you'd pass (f, x) -> map(f, children(x)). Now the question becomes what to call that parameter 😄

@darsnack
Copy link
Member

darsnack commented Jul 1, 2021

Yeah but all the example transforms are just identity selectively applied. Well you convinced me anyways, because I think the API that you are proposing is like the "walk" parameter I asked for before. For example, here I could use this parameter to only walk the prune-able parameters. So, it's more flexible than just conditional reconstruction.

@ToucheSir
Copy link
Member Author

fmap(length, m) (for parameter counting) is an example transform that isn't a selectively applied identity :)

walk isn't a bad name for the parameter. If we're all on the same page about this, I'll switch over the PR to adding that to fmap. We could still have a functorize(f, x; kwargs...) = fmap(f, x; walk=(f, x) -> map(f, children(x)), kwargs...) as well.

@darsnack
Copy link
Member

darsnack commented Jul 1, 2021

Yeah switching the PR to walk sounds good. Maybe we can rename functorize to fmapstructure?

@ToucheSir ToucheSir changed the title Functorize WIP fmapstructure Jul 1, 2021
@darsnack
Copy link
Member

darsnack commented Jul 1, 2021

Maybe add something in the README? Also, I think this can be lifted out of WIP; it's pretty much good to go 😄.

@darsnack
Copy link
Member

darsnack commented Jul 1, 2021

Also should we rename fmap1 while we're at it? Could it be called walkfunctor? Or mapfunctor?

@ToucheSir
Copy link
Member Author

I had to add a better test after finding a bug in the first version (walk wasn't being applied recursively). Also added some docstrings based on the fcollect docstring.

As for what to name fmap1, I'm honestly not sure. walkfunctor or mapfunctor seem like better names for fmap itself honestly. I thought of destructure_map_restructure, fmap_helper, default_fmap_helper, default_walk, etc. (all with and without leading underscores), but none of them really stood out.

@ToucheSir ToucheSir marked this pull request as ready for review July 1, 2021 21:23
@darsnack
Copy link
Member

darsnack commented Jul 1, 2021

Well I came up with walkfunctor, because I plan to refactor FluxPrune.jl to have walkpruneable. Similarly, we could have walktrainable one day. walkleaves, walkchildren, mapleaves, and mapchildren are also options.

@darsnack
Copy link
Member

darsnack commented Jul 2, 2021

Also, _fmap is still better than fmap1 despite being nearly the same.

@darsnack
Copy link
Member

darsnack commented Jul 2, 2021

After sleeping on it, my vote is for default_walk.

@ToucheSir
Copy link
Member Author

Thoughts on leading vs no leading underscore? I couldn't find any reference to fmap1 in the wild, but I imagine we will never want this to leak beyond the internals of Functors.jl.

@darsnack
Copy link
Member

darsnack commented Jul 4, 2021

_default_walk and un-exported is probably best to keep it internal.

@ToucheSir
Copy link
Member Author

Done. Shall we get this merged so that Flux can start using it?

Copy link
Member

@darsnack darsnack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple docstring suggestions, but it looks good otherwise.

src/functor.jl Outdated Show resolved Hide resolved
src/functor.jl Outdated Show resolved Hide resolved
src/functor.jl Outdated

Like [`fmap`](@ref), but doesn't preserve the type of custom structs.

Useful for when the output must be plain-old julia data structures.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we omit this? I'm not sure it's the right wording, since functored structs are still "plain-old Julia data structures."

src/functor.jl Outdated Show resolved Hide resolved
@@ -13,7 +13,7 @@ function makefunctor(m::Module, T, fs = fieldnames(T))
f in fs ? :(y[$(yᵢ += 1)]) : :(x.$f)
end
escfs = [:($f=x.$f) for f in fs]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of no-op line changes. Do you know what that's from?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, my editor may have trimmed existing trailing whitespace automatically on save?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, so we'll keep these

src/functor.jl Outdated Show resolved Hide resolved
Co-authored-by: Kyle Daruwalla <daruwalla.k.public@icloud.com>
@@ -13,7 +13,7 @@ function makefunctor(m::Module, T, fs = fieldnames(T))
f in fs ? :(y[$(yᵢ += 1)]) : :(x.$f)
end
escfs = [:($f=x.$f) for f in fs]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, so we'll keep these

src/functor.jl Outdated Show resolved Hide resolved
src/functor.jl Outdated
Comment on lines 63 to 83
# Examples
```jldoctest
julia> struct Foo; x; y; end

julia> @functor Foo

julia> struct Bar; x; end

julia> @functor Bar

julia> m = Foo(Bar([1,2,3]), (4, 5));

julia> fmap(x -> 2x, m)
Foo(Bar([2, 4, 6]), (8, 10))

julia> fmap(string, m)
Foo(Bar("[1, 2, 3]"), ("4", "5"))

julia> fmap(string, m, exclude = v -> v isa Bar)
Foo("Bar([1, 2, 3])", (4, 5))
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want a custom walk example?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't think of a succinct one that wasn't fmapstructure, so I just added a doctest for that instead. Are there any examples you can think of?

Copy link
Member

@darsnack darsnack Jul 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just something that only walks the children that are arrays of arrays? Or some other simple type constrained walk.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put in an absolutely uninspired example, but it's an example nonetheless. Happy to change it if something more illustrative comes up.

@darsnack
Copy link
Member

darsnack commented Jul 9, 2021

@CarloLucibello I don't have permissions on this branch. Or do we use bors?

@DhairyaLGandhi
Copy link
Member

Thanks all

@DhairyaLGandhi DhairyaLGandhi merged commit 6508da8 into FluxML:master Jul 9, 2021
@DhairyaLGandhi
Copy link
Member

DhairyaLGandhi commented Jul 9, 2021

The name fmapstructure makes it sound like fmap itself doesn't preserve structure, which is incorrect. So a better name might be in order. An fmap is a pretty well established concept (see from Haskell)

@ToucheSir ToucheSir deleted the functorize branch July 9, 2021 03:44
@ToucheSir
Copy link
Member Author

I agree the name isn't ideal, but the intent was to point out that this only preserves the structure (for some handwavy definition of "structure") and not the actual types involved. While fmap has signature Functor f => (a -> b) -> f a -> f b , this would be closer to Functor f => (a -> b) -> f a -> Either HList HMap (which is an invalid signature, but my point is to signal the change from a named type into a generic container).

Another way to look at this is as an instance of a poor man's hylomorphism. This take is appealing because it lets you view fcollect and destructure as catamorphisms (functor itself is basically unfold), and _restructure as an anamorphism (the re function returned from functor being equivalent to fold). The analogy is not perfect because of how different both languages' type systems are, but I think it's a good framework for thinking about the space of operations one could do with a (Functors.jl) functor.

@ToucheSir
Copy link
Member Author

Actually, perhaps a more direct analogy would be that Functors.fmap with walk customization behaves more like traverse than fmap. In this framing, anything that implements functor would be not just a Functor, but a Traversable. fmapstructure would then be closer to calling traverse with a function that spits out (named)tuples (themselves Traversables).

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.

3 participants