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

init keyword for reducec and mapreducec #247

Closed
johnnychen94 opened this issue Apr 22, 2021 · 3 comments · Fixed by #263
Closed

init keyword for reducec and mapreducec #247

johnnychen94 opened this issue Apr 22, 2021 · 3 comments · Fixed by #263
Milestone

Comments

@johnnychen94
Copy link
Member

johnnychen94 commented Apr 22, 2021

The current API is not consistent with reduce/mapreduce.

- reducec(op, v0, c)
+ reducec(op, c; init=zero(eltype(eltype(c))))

- mapreducec(f, op, v0, c)
+ mapreducec(f, op, c; init=zero(eltype(eltype(c)))

Maybe, we can just make mapc/reducec/mapreducec as Colorant methods of map/reduce/mapreduce?

@kimikage
Copy link
Collaborator

I agree with the support of the keyword argument init. However, there is room for debate about compatibility.
If we support multiple input colors, (as the current mapc supports,) there will be some confusion. For example:

mapreducec((a, b) -> max(a, b), *, Gray(1), Gray(0)) # Does `Gray(1)` mean the `v0`?

I don't think this is much of a problem in the current situation. However, issue #207 needs to be considered more carefully.
If the current formats are to be deprecated, we need to prepare for depwarn.

BTW, this isn't an essential issue, but the default value of init seems to depend on the operator op.

julia> mapreduce(identity, +, (1.0, 0.5, 0.25))
1.75

julia> mapreduce(identity, +, (1.0, 0.5, 0.25), init=0)
1.75

julia> mapreduce(identity, *, (1.0, 0.5, 0.25))
0.125

julia> mapreduce(identity, *, (1.0, 0.5, 0.25), init=1)
0.125

@kimikage
Copy link
Collaborator

I also set one of the milestones for v0.13 to be the provision of iterator interfaces (cf. #131 (comment))
In that case, reducec and mapreducec may not be necessary anymore.

julia> components(c::Color3) = (comp1(c), comp2(c), comp3(c)); # substitute a tuple for an iterator object

julia> mapreduce(identity, *, components(RGB(1.0, 0.5, 0.25)))
0.125

julia> mapreducec(identity, *, 1.0, RGB(1.0, 0.5, 0.25))
0.125

@kimikage
Copy link
Collaborator

kimikage commented Apr 27, 2021

Even if we may deprecate reducec and mapreducec in the future, I think it would be beneficial to change their implementation to reduce/mapreduce-based (unless there is a significant performance degradation).

The ability to omit the initial value is more attractive than the ability to explicitly specify the init keyword argument. Shall we keep the current formats and add support for the new formats in ColorTypes v0.12?

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 a pull request may close this issue.

2 participants