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 option for automatic return type inference #2

Closed
wants to merge 3 commits into from

Conversation

RomeoV
Copy link
Contributor

@RomeoV RomeoV commented Jan 29, 2024

Consider after #1.

@carstenbauer
Copy link
Member

As you probably know, Core.Compiler.return_type isn't public API and also might give up (i.e. return Any or similar) at some point.

I agree that automatically figuring out the return type is very convenient, especially in common cases where, e.g., typeof(x) == Float64 and typeof(f(x)) == Float64. OTOH, being explicit about it is clean and stable. The question is if we want to default to clean and stable or convenient. In any case, there should always be an option to provide the output type explicitly (as is the case here).

@MasonProtter
Copy link
Member

MasonProtter commented Jan 29, 2024

I think if end users want to use return_type here that's totally fine and legitimate, but I also think this particular usage of return type is something that we probably shouldn't do in a package because it makes it so that there are observable differences in behaviour if the compiler gives up, which is generally not good (also an easy way to get removed from PkgEval)

StableTasks also uses return_type, but the difference is that it doesn't change the behaviour of your program if type inference fails.

@MasonProtter
Copy link
Member

MasonProtter commented Jan 29, 2024

However, I do have an alternative way we could try to help users avoid writing the type for map, but it'll involve a few more allocations.

Could just do something like

using BangBang: append!!
function tmap(f, A; nchunks)
    tmapreduce(append!!, chunks(A; n=nchunks)) do inds
        map(f, @view A[inds])
    end
end

@carstenbauer
Copy link
Member

I guess this can be closed now that #6 was merged?

@MasonProtter
Copy link
Member

Thank you anyways @RomeoV!

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.

3 participants