Deprecate/remove map(f) and foreach(f)

[tkf]

It might be useful to define curried version of iterator transforms such as Iterators.map(f) = x -> Iterators.map(f, x) (after #34352) and Iterators.filter(f) = x -> Iterators.filter(f, x) (ref #33027, #24990 (comment)). However, since the eager counter part map(f) is currently defined as f(), it may confuse users when the difference between map(f) and Iterators.map(f) are more than laziness.

Can we remove map(f) at some point? (I think whether or not it is desirable to do this within Julia 1.x is up to core devs.) I think it makes sense to also remove foreach(f) when removing map(f) for consistency.

map(f) was added by @stevengj in #17318. The reasoning was that it is better to be consistent with f.(). I agree that considering f.() as a special case of f.(args...) makes sense for broadcasting; i.e., the output dimension of broadcasting is reduce(max, ndims_of_arguments) when ndims_of_arguments is not empty. Since ndims is always a non-negative integer, it is reasonable to use the identity element of max on non-negative integers which is 0, when there is no argument. However, this argument relies on that the "ndims" of the output is flexibly determined (i.e., using the largest ndims) in broadcasting. As this is not the case in map, I think it makes less sense to define map(f) as f(). OTOH, I think curried map is a useful definition.

foreach(f) was added by @JeffBezanson in #13774. It seems there was no discussion for adding foreach(f).

[andyferris]

I've thought the same for a long time. There's a nice example of composition in haskell about how easy it is to do double map. The currying form allows things like:

julia> map(map(iseven), [[1,2,3],[4,5]])
2-element Array{Array{Bool,1},1}:
 [0, 1, 0]
 [1, 0]

julia> [[1,2,3],[4,5]] |> map(map(iseven))
2-element Array{Array{Bool,1},1}:
 [0, 1, 0]
 [1, 0]

I find this kind of thing comes up surprisingly often.

[tkf]

Yet another reason to remove map(f) is that

map(f, itrs...) == collect(f(args...) for args in zip(itrs...))

is not correct anymore for itrs === (). The right hand side calls f infinitely many times (and crash) while the left hand side calls f only once. Similarly, for args in zip(); f(args...); end calls f infinitely many times while foreach(f) calls f only once. (See #35916 (comment) for why this behavior of zip() is correct.)

So the current behavior of map(f) and foreach(f) is arguably wrong.

Of course, currying for map(f) is much better than a crash.

[tkf]

Quoting @StefanKarpinski's comment #31677 (comment)

I think we should do both that and this. We could try it and if it breaks any package, reconsider, but I bet it won't.

So, I'm removing "RFC" from the title and adding help wanted and good first issue tags.

[StefanKarpinski]

I did mark that issue for triage since my opinion isn't really the final word, but if anyone else really objects, they can speak up. @JeffBezanson you got a strong opinion either way about this?

[tkf]

Right, thanks for clarifying it. I just thought people are in more-or-less agreement and creating a PR would be effective to push it forward at this stage. But whoever creating the PR should know the "risk" that it could be rejected (although I think it's very low-risk).

[stevengj]

I don't have strong feelings about this, but it's hopefully unlikely to break anything in practice.

[StefanKarpinski]

The reason I doubt this will break things in practice is that the usual argument for the zero-arg case is that someone may have written generic code where the zero-arg case should work consistently for the sake of corner cases, but I just don't think people are writing much code using map where the number of things being mapped over is variable and includes zero things. Yes, it's theoretically possible, but it just seems kind of unlikely.

[JeffBezanson]

Yeah I agree the current meaning is pretty useless.

[StefanKarpinski]

Splatted maps that could potentially be affected by this:

https://juliahub.com/ui/RepoSearch?q=%5Cbmap%5C%28%5Cs%2a%5Cw%2B%5Cs%2a%5C,%20%5Cw%2B%5C.%5C.%5C.%5C%29&r=true

[JeffBezanson]

There are certainly cases there like map(tuple, args...) that do the right thing for 0 arguments (giving the empty tuple in that case). So we can't conclude that this is useless/unused, so this is quite breaking.

[StefanKarpinski]

Pretty much all the instances of map(f, args...) in the above search show that this corner case is actually useful because they do the right thing.

[StefanKarpinski]

We may need a different way of getting the curried version. We could say mapper(f) and foreacher(f)?

[andyferris]

Hmm... I do have code that this will break.

mapper is kinda cute. Though that implies some interesting ones like filterer, broadcaster and possibly other ones like grouper, innerjoiner, mergewither!.

Ideally though we wouldn’t have to create a new, ever-expanding set of generic functions to support currying. That seems to leave 3 options: syntax (like map(f, _)), having a special signifier (like how we use : for indexing but behaving like the _) that tells the higher order function to return the curried form as a convention, or a new function that “lifts” map etc to a curried version (e.g. curry(map, f)).

[StefanKarpinski]

map(f, ...) could potentially be shorthand for (args...; kws...) -> map(f, args...; kws...).

[LilithHafner]

Triage says we should remove map(f) and foreach(f)

[nsajko]

EDIT: see a more careful analysis below

The Nanosoldier run on #52631 completed. I see at least these three packages that are broken by the change:

1. EasyJobsBase: some of its tests (but not package code, as far as I can tell) depend on the single-argument map method existing
2. SimpleWorkflows: same as EasyJobsBase, also same package author
3. ProgressMeter: a test (but not package code, as far as I can tell) expects for the single-argument foreach method to exist

There's also a few other packages flagged as possible regressions by Nanosoldier, but I couldn't find any mention of "foreach" or "map" in their logs.

[andyferris]

Well that sounds encouraging. I still think this change is a good idea.

[nsajko]

Analysis of possible regressions

The Nanosoldier report flags these packages as potential regressions:

1. EasyJobsBase v0.15.0: "has test failures"
2. SimpleWorkflows v0.29.0: "has test failures"
3. SetBuilders v0.1.3: "has test failures"
4. FloatTracker v1.0.0: "has test failures"
5. ProgressMeter v1.9.0: "Package tests unexpectedly errored"
6. GraphViz v0.2.0: "Package tests unexpectedly errored"
7. GtkReactive v1.0.6: "Package tests unexpectedly errored"
8. OIFITS v1.0.0: "Package tests unexpectedly errored"
9. IntensityScans v0.2.6: "Package tests unexpectedly errored"
10. Pseudospectra v0.2.0: "Tests became inactive"
11. Sinograms v0.4.1: "Tests became inactive"
12. MicroTracker v0.3.1: "Tests became inactive"
13. ManifoldDiff v0.3.10: "Test duration exceeded the time limit"
14. TransitionsInTimeseries v0.1.0: "Test duration exceeded the time limit"
15. Bloqade v0.2.1: "Test duration exceeded the time limit"
16. GMT v1.10.1: "Test log exceeded the size limit"

Testing each package locally, 3ed49fd vs #52631:

Julia Version 1.11.0-DEV.1284
Commit 3ed49fdb3a9 (2024-01-17 06:57 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 8 × AMD Ryzen 3 5300U with Radeon Graphics
  WORD_SIZE: 64
  LLVM: libLLVM-16.0.6 (ORCJIT, znver2)
Threads: 1 default, 0 interactive, 1 GC (on 8 virtual cores)
Environment:
  JULIA_NUM_PRECOMPILE_TASKS = 3
  JULIA_PKG_PRECOMPILE_AUTO = 0

Julia Version 1.11.0-DEV.1285
Commit bf55fde43d* (2024-01-17 09:39 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: 8 × AMD Ryzen 3 5300U with Radeon Graphics
  WORD_SIZE: 64
  LLVM: libLLVM-16.0.6 (ORCJIT, znver2)
Threads: 1 default, 0 interactive, 1 GC (on 8 virtual cores)
Environment:
  JULIA_NUM_PRECOMPILE_TASKS = 3
  JULIA_PKG_PRECOMPILE_AUTO = 0

For each package P, I start by deleting my Julia depot, add the package with add P@v in the Pkg REPL, and test it with test P in the Pkg REPL.

Analysis of each package

1. EasyJobsBase

The test failures are caused by the PR, in particular because of the lack of a
single-argument map method. However, neither this package nor its dependency
Thinkers use either map or foreach anywhere under src/, so the PR seems
to break the package's tests without breaking the package. EasyJobsBase does
use map under test/.

2. SimpleWorkflows

This is depends on EasyJobsBase and shares the author. The tests again fail due
to the lack of a single-argument map method, but, again, neither map nor
foreach are used under src/.

3. SetBuilders

This fails on master, I think the test is flaky.

4. FloatTracker

Flaky test.

5. ProgressMeter

The test failures are caused by the PR, the tests expect single-argument methods
for both map and foreach. The package code itself (under src/) doesn't depend
on the single-argument methods, though.

6. GraphViz

Flaky test.

7. GtkReactive

For Nanosoldier, the testsuite passes on master but fails with the PR. For me,
eight tests fail on both master and with the PR. Maybe someone else should check
this, too. For what it's worth, the package doesn't seem maintained and the Readme
has this note:

NOTE: new projects are encouraged to consider GtkObservables instead.

8. OIFITS

Unrelated error: segfaults on both master branch and PR branch, #52951.

9. IntensityScans

Works for me, so flaky.

10. Pseudospectra

Unrelated error: hangs.

11. Sinograms

I guess this is a flaky hanger just like Pseudospectra, not going to test it.

12. MicroTracker

I guess this is a flaky hanger just like Pseudospectra, not going to test it.

13. ManifoldDiff

Some kind of unrelated error happened during precompilation for Nanosoldier, but
works for me.

14. TransitionsInTimeseries

Some kind of unrelated error happened during precompilation for Nanosoldier, but
works for me.

15. Bloqade

Some kind of unrelated error happened during precompilation for Nanosoldier, but
works for me.

16. GMT

Unrelated error, flaky. Sometimes segfaults, sometimes Julia vomits huge amounts of
GC debugging info after all tests pass.

Conclusion

Making this change would break the test suites of three packages, but it doesn't
seem like it would compromise the functionality of any package.

[nsajko]

PRs to the repos of packages with affected test suites: MineralsCloud/SimpleWorkflows.jl#207, MineralsCloud/EasyJobs.jl#57, timholy/ProgressMeter.jl#291.

[LilithHafner]

Thank you for this @nsajko!