-
Notifications
You must be signed in to change notification settings - Fork 11
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
Custom definition for append!!(_, Init(append!!))
#148
Conversation
Using commit: Add `INIT` that works with arbitrary operators JuliaFolds/InitialValues.jl@b5b0777
Init(append!!)
monoid!!(_, Init(monoid!!))
Codecov Report
@@ Coverage Diff @@
## master #148 +/- ##
==========================================
- Coverage 92.11% 91.98% -0.14%
==========================================
Files 21 22 +1
Lines 444 449 +5
==========================================
+ Hits 409 413 +4
- Misses 35 36 +1
Continue to review full report at Codecov.
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/BangBang.jl/BangBang.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/BangBang.jl/BangBang.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/BangBang.jl/BangBang.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
Using commit: Fix ambiguity of promote_rule in Julia 1.0 (< 1.3) JuliaFolds/InitialValues.jl@4ecc368
monoid!!(_, Init(monoid!!))
append!!(_, Init(append!!))
Benchmark resultJudge resultBenchmark Report for /home/runner/work/BangBang.jl/BangBang.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/BangBang.jl/BangBang.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/BangBang.jl/BangBang.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
Benchmark resultJudge resultBenchmark Report for /home/runner/work/BangBang.jl/BangBang.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/BangBang.jl/BangBang.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/BangBang.jl/BangBang.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
|
This introduces two recent important updates in BangBang: * Custom definition for `append!!(_, Init(append!!))` JuliaFolds/BangBang.jl#148 (requires v0.3.23) * Fix widening path of `materialize!!` JuliaFolds/BangBang.jl#154 (requires v0.3.24)
This introduces two recent important updates in BangBang: * Custom definition for `append!!(_, Init(append!!))` JuliaFolds/BangBang.jl#148 (requires v0.3.23) * Fix widening path of `materialize!!` JuliaFolds/BangBang.jl#154 (requires v0.3.24)
Using the default approach
InitialValues.@def_monoid monoid!!
was problematic because definingmonoid!!(::CustomType, x)
introduced method ambiguities. It could be solved semi-automatically by@disambiguate append!! CustomType
but it is very ugly and very problematic for the extensibility of the method.Furthermore, the definition
that is generated by
@def_monoid
is not appropriate ifsrc
should be consumed by the timeappend!!
is called (e.g., https://discourse.julialang.org/t/38845/3). This is mostly the case since the returned value ofmonoid!!(init, src)
would be used as the first argument (i.e., it would be mutated) in the "next iteration" in reduction code idioms. That is to say,monoid!!
should not assume the ownership of the second argument.The method ambiguity problem is solved by introducing a "dispatch pipeline"; i.e., a
CustomType
implementer can overload__monoid!!__
instead ofmonoid!!
.The memory ownership problem is solved by defining InitialValues.jl interface manually. For example,
append!!(::InitAppend!!, src)
now callscopymutable(src)
.Alternative approach
For
append!!
, an alternative approach considered is to create a customEmptyCollection
type that behaves like "an" identity element ofappend!!
. This may be possible by allowingInitialValues.Init(::typeof(monoid!!))
overload. This approach has some nice properties:No need to define
append!!(::Any, ::InitType)
as it would work via the default implementation ofappend!!
based on iterator. (The flip-side is that a bug could be hidden if the init object is fed to other functions likecollect
. But this is not a serious problem.)By using the trait mechanism and defining
NoBang.append
,append!!
would behave as expected without any "surface" dispatches.A major downside of this approach is that the fold code does not have a canonical way to check if
monoid!!
is ever called. If overloadingInitialValues.Init
were supported, the fold implementers cannot simply callacc isa InitialValue
to check ifacc = monoid!!(acc, x)
was called at least once.Commit Message
Custom definition for
append!!(_, Init(append!!))
(#148)Implement a better fix for the method ambiguity problem of
append!!(_, Init(append!!))
. Also fix the ownership problem ofappend!!(Init(append!!), xs)
by copyingxs
.