-
Notifications
You must be signed in to change notification settings - Fork 1
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
Clean up apply! code #25
Changes from all commits
88a6441
7bd767a
ab8e001
1ffcbca
eafb982
cfaab06
6d438f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,11 +43,6 @@ function _apply(x, P::Periodic{T}; kwargs...) where T <: Period | |
map(xi -> _periodic(P.f, xi, P.period, P.phase_shift), x) | ||
end | ||
|
||
function _apply!(x::AbstractArray{T}, P::Periodic; kwargs...) where T <: Real | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is informative benchmarking, but I was curious about the effect. This branch:
Why are there more allocations? What are approaches to reducing allocations? I'm not familiar with it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not sure either, I gotta look into it. One thing to note however, is that when using BenchmarkTools I think you have to interpolate the function args as this can affect the result https://github.com/JuliaCI/BenchmarkTools.jl#quick-start (note: this doesn't apply to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just to note that with julia> @benchmark Transforms.apply!($x, $p)
BenchmarkTools.Trial:
memory estimate: 22.89 MiB
allocs estimate: 7
--------------
minimum time: 5.029 ms (0.00% GC)
median time: 6.940 ms (16.92% GC)
mean time: 6.427 ms (19.47% GC)
maximum time: 9.906 ms (32.37% GC)
--------------
samples: 778
evals/sample: 1 |
||
x[:] = _apply(x, P; kwargs...) | ||
return x | ||
end | ||
|
||
""" | ||
_periodic(f, instant, period, phase_shift=Day(0)) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why @views at this level? Is it because it should be done whenever iterating the data, and this is the only internal _apply that does so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily every time but since we are indexing into
results
and assigning new values I figured using@views
might help here. I could be wrong though.