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

Add Iterators.map #34352

Merged
merged 13 commits into from
Jun 18, 2020
Merged

Add Iterators.map #34352

merged 13 commits into from
Jun 18, 2020

Conversation

tkf
Copy link
Member

@tkf tkf commented Jan 12, 2020

As @JeffBezanson mentioned in #34033 (comment), it makes sense to have Iterators.map. This PR adds it (with some tests).

NEWS.md Outdated
@@ -19,7 +19,7 @@ Build system changes

New library functions
---------------------

* `Iterators.map` is added ([#34352]).
Copy link
Member

Choose a reason for hiding this comment

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

Might want to indicate what it does.

@@ -22,7 +22,27 @@ import .Base:
getindex, setindex!, get, iterate,
popfirst!, isdone, peek

export enumerate, zip, rest, countfrom, take, drop, takewhile, dropwhile, cycle, repeated, product, flatten, partition
export enumerate, zip, rest, countfrom, take, drop, takewhile, dropwhile, cycle, repeated, product, flatten, partition, map
Copy link
Member

Choose a reason for hiding this comment

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

It should not be exported, since it will conflict with Base.map.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a little sad that we didn't stick with the convention that the capitalized type name is the lazy construct while the lowercase function name is the eager construct. IIRC, we had agreed on that around 0.2 era but then didn't end up sticking to it.

@tkf
Copy link
Member Author

tkf commented Jan 14, 2020

I addressed the points in the review. Now Iterators.map is not exported and NEWS.md has a bit more description.

@bramtayl
Copy link
Contributor

bramtayl commented Jan 14, 2020

Can we have generators lower to Iterators.map instead of Base.Generator?

@tkf
Copy link
Member Author

tkf commented Jan 14, 2020

That sounds like a great idea. Same holds for Iterators.filter, Iterators.flatten, and Iterators.product. Can you open a new issue to track the idea?

@bramtayl
Copy link
Contributor

Sure #34368

NEWS.md Outdated Show resolved Hide resolved
Co-Authored-By: Stefan Karpinski <stefan@karpinski.org>
@StefanKarpinski
Copy link
Member

Seems to have trailing whitespace.

@tkf
Copy link
Member Author

tkf commented Jan 15, 2020

buildbot/whitespace_linux32 (https://build.julialang.org/#/builders/150/builds/4226) seems to fail due to network failure (?):

fatal: git fetch-pack: expected ACK/NAK, got 'ERR upload-pack: not our ref 7ad6d9c178229d71962daa2913aee8b21865c175'
fatal: The remote end hung up unexpectedly

@oxinabox
Copy link
Contributor

oxinabox commented Feb 3, 2020

This has been in IterTools.jl forever as imap
I feel like adding every function from IterTools.jl into Base.Iterators is a kind of bad path.

Julia since the 0.7 release (and even before) has pushed towards functionality should live in packages, not that standard library.

In particular, Base.Iterators provides the minimal set of iterators to implement the rest of the standard libraries.

I also have this complaint about #34033 but I didn't see it before it was merged.

@tkf
Copy link
Member Author

tkf commented Feb 3, 2020

In particular, Base.Iterators provides the minimal set of iterators to implement the rest of the standard libraries.

#34033 doesn't seem to imply this.

@oxinabox
Copy link
Contributor

oxinabox commented Feb 3, 2020

As I said, I would have called #34033 out if I had have seen it before it was merged.

@bramtayl
Copy link
Contributor

bramtayl commented Feb 3, 2020

I support not adding unnecessary stuff to Base.

This issue isn't about adding a new iterator, just an interface to one that already exists (namely, Base.Generator).

@tkf
Copy link
Member Author

tkf commented Feb 4, 2020

Yes, that's a good point. It'd be nice to have a public and functional interface for constructing Base.Generator.

Copy link
Contributor

@stev47 stev47 left a comment

Choose a reason for hiding this comment

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

I support this change, since it essentially just adds an alias that is very intuitive.

@testset "Iterators.map" begin
@test collect(Iterators.map(string, 1:3)::Base.Generator) == map(string, 1:3)
@test collect(Iterators.map(tuple, 1:3, 4:6)::Base.Generator) == map(tuple, 1:3, 4:6)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be good to test that Iterators.map can be applied to infinite iterables such as Iterators.cycle(1:3) and has the expected behaviour for the first n iterates.

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the reason why testing infinite iterators is interesting here? I'm testing these two invocations since Base.Generator has different methods for them. So, I think it's somewhat meaningful to make sure we can hit these methods. Other than that, I don't think we need to add tests specially for Iterators.map and they can be done in Base.Generator. I think it makes sense to rename direct uses of Base.Generator in Base/stdlib. But I think it'd be better to do this in a separate PR to make this PR more "atomic".


!!! compat "Julia 1.5"
This function requires at least Julia 1.5.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe cross-reference with the docstring of Base.Generator

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think Base.Generator is included in the documentation. This is partially why I want to add Iterators.map.

@stev47
Copy link
Contributor

stev47 commented May 3, 2020 via email

@stev47
Copy link
Contributor

stev47 commented May 3, 2020 via email

@tkf
Copy link
Member Author

tkf commented May 3, 2020

It tests the laziness of Iterators.map since non-lazy map can't handle infinite iterators.

Existing tests already check that Iterators.map returns Base.Generated.

It has a docstring on Julia 1.4.1 at least.

Having a docstring does not mean it's a public API. See #35715

@stev47
Copy link
Contributor

stev47 commented May 3, 2020 via email

@tkf
Copy link
Member Author

tkf commented May 3, 2020

There is a discussion on the extension of Iterators.map #34368. Documenting what it returns limits such extension. In general, I think it's better to document how a certain object behaves rather than what it actually is.

@tkf
Copy link
Member Author

tkf commented May 3, 2020

I resolved the merge conflict and CI is all green now. Can someone merge this?

(ping @StefanKarpinski @JeffBezanson)

@tkf
Copy link
Member Author

tkf commented Jun 7, 2020

(ping @StefanKarpinski @JeffBezanson)

@StefanKarpinski StefanKarpinski added the triage This should be discussed on a triage call label Jun 11, 2020
@JeffBezanson JeffBezanson merged commit 1f8b442 into JuliaLang:master Jun 18, 2020
@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Jun 18, 2020
@tkf tkf deleted the Iterators.map branch June 18, 2020 19:01
@andyferris
Copy link
Member

This is exciting!

I'd note that the NEWS entry seems to imply that (f(x) for x in iter) would lower to Iterators.map(f, iter), but that doesn't appear to be implemented here. (I realize there is the discusison at #34368).

@andyferris
Copy link
Member

Hmm... or maybe I'm just reading that sentence wrong?

@tkf
Copy link
Member Author

tkf commented Jun 18, 2020

My intention was that both (f(x) for x in iter) and Iterators.map(f, iter) evaluate to the same thing. But yeah, I think lowering the comprehensions to Iterators.map etc. would be great.

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.

7 participants