Skip to content

Conversation

@robertzk
Copy link
Contributor

@robertzk robertzk commented Mar 1, 2015

No description provided.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

Copy link
Member

Choose a reason for hiding this comment

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

Clearly this has to change. I don't know much Python so would be interested to hear the answer myself: is it certain that acc.add(x) evaluates to true so that f(x) is always evaluated?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks to me like acc.add(x) will always evaluate to None [1], so the function will never evaluate f(x). A hacky way to fix this may be to change the lambda to

lambda x: not acc.add(x) and f(x)

because not None evaluates to True, but this certainly won't make things more clear for the reader. The best fix would be do define a separate function

def g(x):
    acc.add(x)
    return f(x)

data.map(g)

Copy link
Member

Choose a reason for hiding this comment

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

@robertzk what do you think of that fix? + @ilganeli just because it looks like you created the example, although this bit may have been inadvertent copy and paste.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the separate function; there really is no good way to write multi-line lambdas in Python.

@robertzk
Copy link
Contributor Author

robertzk commented Mar 6, 2015

@srowen What do you think of this?

@srowen
Copy link
Member

srowen commented Mar 6, 2015

Certainly looks like something that stands a change of working, and approved by @laserson. I'll wait a little for any other comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks fine, but if you want a crazy one-liner:

data.map(lambda x: (acc.add(x), f(x))[1])

Don't do that at home, though. We already have Perl for that kind of shenanigans.

@davies
Copy link
Contributor

davies commented Mar 6, 2015

Nice catch, LGTM, thanks!

@srowen
Copy link
Member

srowen commented Mar 7, 2015

Oh I realized the variable name is accum in this example and needs to be fixed from acc on usage. I can fix on merge.

@davies
Copy link
Contributor

davies commented Mar 7, 2015

@srowen Good catch, I saw that, then forget it immediately, really need to take a rest..

@srowen
Copy link
Member

srowen commented Mar 7, 2015

Heh, actually the Scala and Java examples are wrong too. They should be respectively:

val accum = sc.accumulator(0)
data.map { x => accum += x; f(x) }
Accumulator<Integer> accum = sc.accumulator(0);
data.map(x -> { accum.add(x); return f(x); });

... and I can port the spelling fix to their comments too.

@asfgit asfgit closed this in 48a723c Mar 7, 2015
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