keys(::Generator) pass through

[rapus95]

Let keys pass through to allow lightweigth/lazy find* and arg* functions
Fixes #34674

[rapus95]

@fredrikekre
What would be the intended tests here?
is that already enough?

@test keys(x^2 for x in 1:10) == keys(1:10)

[KlausC]

As a test maybe more illustrative:

@test keys(x^2 for x in -1:0.5:1) == 1:5

I propose to put that into test/functional.jl

[StefanKarpinski]

Triage finds it a little weird but approves since it makes find work and doesn't seem harmful. Would be good to add @KlausC's test and a NEWS entry, however. A use case would also be good.

needs tests!

[rapus95]

@mbauman sure, got interrupted by something else after rebasing such an old branch for the first time 😇
For the NEWS entry, into which section should this go?

Edit: for the usecase:

julia> findmax(map(x->x^2, 20:30))
(900, 11)

julia> findmax(x^2 for x in 20:30)
ERROR: MethodError: no method matching keys(::Base.Generator{UnitRange{Int64},var"#7#8"})

After this change, the latter produces the same result as the eager map variant:

julia> findmax(x^2 for x in 20:30)
(900, 11)

By that it allows to findmax without allocating the whole intermediate map output and thus becomes lazy aswell.

[thofma]

Kind of fixes #23680

Instead of indmin I just have to use findmin.

[rapus95]

@thofma once this lands, you can just use indmin again since keys for Generator won't be missing anymore.

[rapus95]

@mbauman well since I guess 1.5 is already pinned now (branched away from master), but to not to add it to the wrong news file, this'll go into 1.6, won't it? (so I can add a news entry straight to the 1.6 news file)

I'd put it into the "standard library changes" section.

[tkf]

I think keys, values and getindex should satisfy the invariance

isequal(collect(values(A)), [A[k] for k in keys(A)])

So, I'm against this patch unless values and getindex are defined for Generator. See #36175.

[rapus95]

bump for 1.6 👀

[rapus95]

@fredrikekre would you remove the "needs test" label please. (Since there already is one). And can you add it to a 1.6 milestone? (so that it won't go unnoticed upon freezing, especially since triage already accepted the standalone and #37648 holds the getindex for generators)

[oscardssmith]

Rerunning ci, then good to merge in my view.

[oscardssmith]

@StefanKarpinski me closing and re-opening doesn't seem to have trigered CI. Can you make it re-run?

[rapus95]

StefanKarpinski me closing and re-opening doesn't seem to have trigered CI. Can you make it re-run?

@oscardssmith To me it seems like buildbot was triggered and currently is only missing tester_macos64 and tester_win32. analyzegc_linux64 and package_freebsd64 failed, the rest completed successfully.

[oscardssmith]

oh yeah. Sorry for the noise. It might be worth rebasing this onto master so we can get a fully green CI.

[musm]

This looks like it still has a triage label, was this approved? Typically, the triage label would be removed before we allow a PR to be merged.

[oscardssmith]

Triage approved back in March. I think it was re-added to consider back-porting once merged, but I'm not sure.

[musm]

To be sure, it would be best to hold off then until triage-reapproves and once the requested review is approved.

[JeffBezanson]

Triage is ok with this again. I came around since it basically seems consistent with axes, which we already define here. We don't want to backport it but it can be merged on master.

[rapus95]

Ok that's fine, then we should be able to discuss the long-term meaning of Generator and thus, if we also want to include the other issues/PRs (#37648, #34368) which would lead to a smoother feature addition in 1.7

[simeonschaub]

Thank you for the contribution! Sorry this took so long. In the future, also feel free to bump PRs every once in a while, because otherwise things sometimes get forgotten.