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

Replace flatmap with a flatten method #47327

Closed
wants to merge 1 commit into from
Closed

Conversation

LilithHafner
Copy link
Member

From @thchr and @mcabbott's post-merge comments on @nlw0's PR #44792

I.e. this seems to just add another short verb for a nearly trivial composition of two more basic verbs?

We have sum(f, x) as a more efficient sum(map(f, x)), but no summap symbol. Likewise all(f, x) instead of allmap, and so on...

I would also support removing the function entirely, but triage seems to oppose that

Long story short, triage yesterday was ok with flatmap

@LilithHafner LilithHafner added this to the 1.9 milestone Oct 26, 2022
@LilithHafner
Copy link
Member Author

Cross ref to a larger PR that also includes these changes #45985

@mcabbott
Copy link
Contributor

mcabbott commented Oct 26, 2022

Maybe worth noting that no other Iterators functions follow the sum(f, x) pattern. Does this PR raise the expectation that others should?

Iterators.repeated(randn, 2) already means something.

Iterators.product(atan, 1:3, 4:5) might be useful, and presently collects to an error, but would have to dispatch on (f::Function, x...). (As does this PR's flatten I see.)

"""
flatten(f, iterators...)

Create a lazy flattened mapping. Equivalent to `flatten(Iterators.map(f, iterators...))`.
Copy link
Member

Choose a reason for hiding this comment

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

It's not entirely equivalent to Iterators.map because the function argument that accepts is untyped whereas the one this accepts is constrained to be a Function. How bad would it be to do something like

flatten(f, itrs...) = applicable(iterate, f) ? flatten((f, itrs...)) : flatten(map(f, itrs...))

? An approach like this would end up "preferring" iterating in the case that the user passes in some godforsaken object that is both iterable and callable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Flatten only accepts a single argument right now, so it would be unambiguous to define flatten(f, itrs...) = flatten(map(f. itrs...)), but I suppose I wanted nicer error messages when someone calls flatten([1,2,3], [4,5,6]).

Copy link
Member

Choose a reason for hiding this comment

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

Oh my mistake, I could have sworn there was a vararg method.

@LilithHafner
Copy link
Member Author

Maybe worth noting that no other Iterators functions follow the sum(f, x) pattern. Does this PR raise the expectation that others should?

Yeah... I'm not a huge fan of adding this method, but I prefer it over the flatmap function because that very criticism is much stronger in that context: no other functions at all follow the fuctionmap(f, x) pattern and I don't want to raise the expectation that productmap(f, x, itrs...) = product(map(f, itrs...)).

OTOH, the (function ∘ map) pattern is already supported, we have (flatten ∘ map), (product ∘ map), and even (map ∘ repeated)`.

@rfourquet
Copy link
Member

rfourquet commented Oct 27, 2022

no other functions at all follow the fuctionmap(f, x) pattern

I would think that mapreduce is close enough to this pattern, which btw added an item to my todolist: "rename flatmap to mapflat" ;-p

@nlw0
Copy link
Contributor

nlw0 commented Oct 27, 2022

I can't understand why opening this new PR. I can understand it's a smaller patch, but we could settle over #45985 the question of whether doing this is desirable. I am opposed to it (both this and #45985). I do see it's a nifty idea, but I'd much rather have separate flatten and flatmap methods. I opened the PR to generate more discussion and hopefully get a message from core developers about this, since I've been continuously challenged about my proposal of having any kind of flatmap function, under any name. As sure as the sun will come up tomorrow, you can bet someone will pop up and say "you can just use a for-loop" or whatever.

Triage was fine with having an Iterators.flatmap definition, with that name. It was merged. Now some people are mad about it for multiple reasons. And I have my own goal of bringing flatmap with arrays to Base. I'm trying to have a dialog, but it doesn't seem to go anywhere. What are we expecting, that triage will say "no, keep it like that" or "oh yeah, we regret it"? I suppose it might help a bit, maybe it's the message I'm hoping for, but not sure it would really be a progress as a community.

I made and entire short presentation at JuliaCon basically bacause of this, have you seen it? I try to make the case for flatmap there, among other things. Maybe you could give me some comments about the presentation, and not merely about my short and controversial PRs? https://www.youtube.com/watch?v=2VccdiX-9Tg

@nlw0
Copy link
Contributor

nlw0 commented Oct 27, 2022

Maybe worth noting that no other Iterators functions follow the sum(f, x) pattern. Does this PR raise the expectation that others should?

This sounds like a slippery-slope fallacy. Should every single "reduce" style function be able to take a function argument like map? I don't think so. Should this be done for some very common cases? Maybe yes, I do think so. I think sum(f,A) is alright. I use it sometimes, and I like it. How many times do you need to use mapreduce(f, +, A) before you decide it's used frequently enough that you should have it defined?

Same thing for flatmap, it's used (or could be used if it existed) frequently enough to be defined. And let me point it out once again, it's present in the language in the form of the comprehension [y for x in A for y in f(x)]. Also, because it's merely flatten ∘ map like everybody knows, it's just natural to define it so that it simply moves all parameters to map. No reason to argue in a flatmap PR what should be the API for map. sum is not naturally defined over map, but reduce. That's an extra feature from someone who said "hack, let's just build it on top of mapreduce". Completely different discussion.

@JeffBezanson
Copy link
Member

I think flatmap is fine --- it's a standard functional programming term.

The methods we have that add a function as the first argument are kind of ugly honestly, and just there as an occasional convenience. A couple concrete problems: It's awkward for functions like flatten that take any number of arguments of various types. In those cases it's easier to understand if the arguments are treated uniformly. Also, there are callable things other than Functions, so the definition doesn't really work perfectly.

@mcabbott
Copy link
Contributor

It's awkward for functions like flatten that take any number of arguments of various types.

Note that flatten takes one argument: #47327 (comment)

@LilithHafner
Copy link
Member Author

I think flatmap is fine --- it's a standard functional programming term.

Okay

@ararslan ararslan deleted the revise-flatmap branch October 28, 2022 15:30
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.

6 participants