-
Notifications
You must be signed in to change notification settings - Fork 3
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
Multiple updates #1
base: main
Are you sure you want to change the base?
Conversation
Any type can support the function call operation.
Commit c4ff1e introduces a dependency on Julia 1.2
ftcall(f, ft::FrankenTuple{Tuple{},(),Tuple{}}) = f() | ||
|
||
# NOTE: this method signature makes sure we don't define map(f) | ||
function Base.map(f, ft::FrankenTuple, fts::FrankenTuple...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this definition of map
really jives with what map
does for other collections, since iteration of FrankenTuple
s provides the elements in sequence rather than treating the named and unnamed parts separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I think that might be okay. I wonder if perhaps you expect isequal(map(f, c), map(f, collect(c)))
(which I kind of expect as well). However that's already not the case for NamedTuple
(second test below):
using FrankenTuples
function _test_map(f, c)
(map(f, c), map(f, collect(c)))
end
@show _test_map(sqrt, (1, 4))
@show _test_map(sqrt, (a=9, b=16))
@show _test_map(sqrt, @ftuple (1, 4, a=9, b=16))
_test_map(sqrt, (1, 4)) = ((1.0, 2.0), [1.0, 2.0])
_test_map(sqrt, (a = 9, b = 16)) = ((a = 3.0, b = 4.0), [3.0, 4.0])
_test_map(sqrt, @ftuple((1, 4, a = 9, b = 16))) = (FrankenTuple((1.0, 2.0), (a = 3.0, b = 4.0)), [1.0, 2.0, 3.0, 4.0])
I think the right "invariant" is
function test_map_collect_commute(f, c)
isequal(
collect(map(f, c)),
map(f, collect(c))
)
end
@show test_map_collect_commute(sqrt, (1, 4))
@show test_map_collect_commute(sqrt, (a=9, b=16))
@show test_map_collect_commute(sqrt, @ftuple (1, 4, a=9, b=16))
and it gives
test_map_collect_commute(sqrt, (1, 4)) = true
test_map_collect_commute(sqrt, (a = 9, b = 16)) = true
test_map_collect_commute(sqrt, @ftuple((1, 4, a = 9, b = 16))) = true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ararslan What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see that you've just recently cherry picked some of the commits from this PR, and addressed some of the other commits too. Great!
Is map
the only thing that this PR would add, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, just wanted to get the repo into a better state then discuss/ruminate on map
separately. I made sure to preserve your authorship for things you fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed, and I appreciate it :)
If you'd prefer smaller PRs, I can break it up.
map
onFrankenTuple
sftcall
works for anything that may support function calls, not just<:Function
hasmethod
to use code inBase
that came out of this package to begin with!