-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Don't hardcode that certain types are leaves in functor implementation #58
Comments
Breaking changes aside, I don't see any major issues with this. Functors.jl is in a weird place because it was designed to be used as part of Flux's module system, yet was written as if it were a generic struct traversal library. The upside is that more libraries outside of FluxML have been able to adopt it, but the downside is confusion from some parts seeming too task-specific (e.g. saying all numeric arrays are leaves) and others seeming too general (how many people know what |
Maybe we should allow users to pass their own |
One option is to build the default |
https://github.com/chengchingwen/StructWalk.jl allows to define customized walks |
I would close this as not planned, since people can just handle numerical arrays or use StructWalk.jl instead. |
Motivation and description
It doesn't seem right to hardcode that e.g.
AbstractArray{<:Number}
is a leaf via making the functor for this type never recurse into its children. This makes it harder when the user wants to, e.g. apply a function to every scalar leaf without worrying about arrays.Possible Implementation
The
exclude
function already has the ability to stop higher up in the tree. So perhaps all leaf information should be encoded in theexclude
function: if we want to stop at a node, simply don't callfunctor
on it in the first place. Then, maybe:functor
should search for children as aggressively as possible, not stopping e.g. atAbstractArray{<:Number}
@leaf AbstractArray{<:Number}
would directly affect the defaultisleaf
functionality, which is used inexclude
, so the previous behaviour would apply by default.The text was updated successfully, but these errors were encountered: