Supporting map and friends

[swt30]

I don't have any experience writing macros yet, but is it possible to let @showprogress support functions like map (and foreach, which has recently been added to Julia master)? I guess you could rewrite

@showprogress map(f, itr)

as something like

p = Progress(length(itr))
map(x -> (next!(p); f(x)), itr)

although I don't know if you need extra precautions in case itr doesn't have a length defined for whatever reason.

[garrison]

I'm hesitant to get into the business of overriding certain functions like this, but that is just my opinion and @timholy may differ.

That said, maybe there could be a convenience function that takes f and p and wraps it like you do.

[swt30]

I did think it seemed weird to be rewriting the function argument f like that. The anonymous function will also slow things down inside the map--at least until they are made faster, which I hear is coming soon.

[timholy]

For the time being, I agree with @garrison, although as you say that may change with fast closures. Eliminating that performance bottleneck will, I suspect, have pretty dramatic (and to me somewhat unpredictable) consequences for how people write julia code.

[garrison]

at least until they are made faster, which I hear is coming soon.

I haven't been paying close attention lately; do you have a link to progress on this front? I agree with @timholy that this could have dramatic consequences for how people write Julia code.

[timholy]

JuliaLang/julia#13412

[tomasaschan]

I am, in principle, very much for the idea of supporting @showprogress map(f, A) and similar constructs, when (but not before) fast anonymous functions land.

I have no arguments against re-writing or wrapping the user-provided function f, but it feels very counter-productive to re-write it to something that's bound to be much slower than the original. @showprogress is most likely to be used for operations that are already slow - making them slower for reasons that won't be apparent to most users is going to be a huge turn-off.

That said, if there was a PR for this functionality (with tests), I wouldn't hesitate to hit "merge" the same day a Julia version with fast anonymous functions is released.

[swt30]

It depends on the use case too: the extra cost of the anonymous function will hardly matter if you are using this to track an iteration over a few big blocks of work rather than many small ones. Anyway, I have been looking for an excuse to learn how to write macros, so I think I might give this a crack :)

[garrison]

I am, in principle, very much for the idea of supporting @showprogress map(f, A) and similar constructs

I should have been more clear before, but what I do not like about this is that the macro is then responsible for recognizing certain functions like map and replacing them with progress-enabled counterparts. On Julia 0.4, methods(map) returns 33 functions. How could we ever be confident that we overrided all of them correctly? The potential for breakage here is too large in my opinion. Instead of a macro, we should provide a function that does this; something like

progressmap(f, itr)

[swt30]

Does the macro see the individual methods, or just the name "map"? If the latter, wouldn't it work anyway - provided only that the particular method being called adheres to the (f, itr) argument order?

If we wrote a separate progressmap function, could you then have

@showprogress map(args...)

expand to

progressmap(args...)

or is that running into the same problems? Mostly I ask because the macro form is very friendly, similar to @show for quick debugging.

[garrison]

The macro really just sees the symbol :map.

[tomasaschan]

On Julia 0.4, methods(map) returns 33 functions. How could we ever be confident that we overrided all of them correctly?

All of them, with one single exception, could be formulated as map(f, args...) = ... where f is something that supports doing f(x...) on x from something conceptually equivalent of zip(args...) (the exception begin map for zero-argument functions, defined here).

I understand that as a deeply, if only conventionally, rooted contract of the map function. Given that it's so (and that the precautions against itr not supporting length that @swt30 mentioned in the original post are actually taken), I think there's very little that can go wrong without a much larger problem in contract breakage from the map function itself. (In other words, I'd consider any map implementation broken if it doesn't support a macro like this.)

[timholy]

It's much more complicated than that. Check base for the source code for the implementation of map; it deals with empty containers, poorly-typed containers, functions that are type-unstable, etc. and yet return a container with sensible type.

[swt30]

Git etiquette question: I am putting together a pull request to try using @showprogress to rewrite the function argument to map as above, so we can see if it's a terrible idea or not. But first I refactored @showprogress a bit without altering the current functionality. Should that be a separate pull request?

[garrison]

Hard to say. I would make it a separate commit but keep it in the same pull request for now. It can always be made its own pull request later if it seems like a reasonable contribution and the second commit is not. (If instead you make the refactor its own pull request, you'll be forced to wait on it being merged before you can submit the second. I guess you could always make a pull request to the branch on which you are working, but that sounds complicated.)

[tomasaschan]

(@garrison As long as there's two commits, there can be two PR's, and one doesn't have to be merged for the other to be filed. What would happen, though, is that before the first one is merged, the second one would appear to contain commits from both of them. Starting to derail here...)

@swt30 As @garrison says, the best option is to create a branch with the refactor and the map support in clearly separated commits. If you feel that the refactor is a step forward in itself, and not just a stepping stone to enable the map support (but pointless if we don't want it), file two PR's, and indicate in the latter that it's based on the former.

[vtjeng]

@timholy what's the best way for me to monitor progress for a map? Do I want to be re-casting it as a for loop? (I'm assuming based on the age of this PR that direct support for a map isn't likely to land soon).

[martinholters]

Can't you just manually do the rewrite as proposed above?

[vtjeng]

@martinholters I could. I was concerned about the possible performance issues highlighted that then seem to get resolved here but I should have just tried it first to see whether it slows down my code significantly.

[zsunberg]

Working on this in #108

[dlfivefifty]

Is supporting broadcast in the works?

[timholy]

I don't know of anyone working on it now.

[Dijkie85]

Is supporting broadcast in the works?

Any chance of implementing this feature?