Skip to content

add getkeypath and haskeypath #76

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

Merged
merged 5 commits into from
Apr 2, 2024
Merged

add getkeypath and haskeypath #76

merged 5 commits into from
Apr 2, 2024

Conversation

CarloLucibello
Copy link
Member

@CarloLucibello CarloLucibello commented Mar 11, 2024

Some convenience functions for KeyPath on top of #74 so that we can have

x = Dict(:a => 3, :b => Dict(:c => 4, "d" => [5, 6, 7]))
kp = KeyPath(:b, "d", 2)
@assert haskeypath(x, kp)
@assert getkeypath(x, kp) == 6

The possibly controversial part of this PR is that we define indexing with a KeyPath for @functorized types.
This would be no longer possible if we decide to functorize everything by default as proposed in #49, so maybe better leave the indexing part out?

TODO

@CarloLucibello CarloLucibello force-pushed the cl/keypath2 branch 2 times, most recently from ec8dcb9 to e7b69d0 Compare March 11, 2024 16:50
@CarloLucibello CarloLucibello changed the base branch from cl/keypath to master March 11, 2024 16:52
@CarloLucibello CarloLucibello changed the title add getkeypath add getkeypath and haskeypath Mar 11, 2024
@ToucheSir
Copy link
Member

What's your thought on extensibility for arbitrary types? Do we care about that or is it not a problem because functor is always called first? If we do care, what's the API for extension? Would it be worth piggybacking off an existing library/API like Accessors.jl for that purpose?

@CarloLucibello
Copy link
Member Author

I'm not sure what you mean, getkeypath and haskeypath do not rely on functor and can be used on arbitrary types.

The convenience of using indexing x[kp] instead of getkeypath is something provided by the @functor macro, but I think that maybe tying like that orthogonal functionalities is better avoided, especially since with indexing are adding little extra value.

@ToucheSir
Copy link
Member

ToucheSir commented Mar 13, 2024

I'm not sure what you mean, getkeypath and haskeypath do not rely on functor and can be used on arbitrary types.

One example I thought of is types like Tangent from ChainRules which try to hide their internal details and ask users to call getproperty instead. Since Optimisers claims to work with these structural tangent types, it would be nice if the keypath functions treated them the same way as, say, a NamedTuple with the same fields. Presently, I think an extra key would be required on the keypath for each nested Tangent to access its internal backing field.

@CarloLucibello
Copy link
Member Author

ok now I understand.

As you said maybe we don't have to care because in a recurrence one calls functor and constructs the KeyPath object along the way. If needed we can point users to overload _haskey and _getkey for their types.

Or we can tie these new methods to functor and do something like

_haskey(x, k::Symbol) = k in children(x)

function _getkey(x, k::Symbol)
    assert _hasjey(x, k)
    return getproperty(x, k)
end

I'm not sure which one is better.

@ToucheSir
Copy link
Member

Do you think we could make the keypath functions use getproperty and propertynames instead of getfield and fieldnames? I can't think of any cases where that would fail currently, and it would align with how .field access lowers to getproperty.

@CarloLucibello
Copy link
Member Author

I didn't know about the existence of propertynames. Yes it seems the sensible thing to do, I'll change the implementation.

@CarloLucibello
Copy link
Member Author

ready to go

@CarloLucibello CarloLucibello merged commit bccabfc into master Apr 2, 2024
@CarloLucibello CarloLucibello deleted the cl/keypath2 branch April 2, 2024 12:39
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