general cleanups #536
general cleanups #536
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #536 +/- ##
==========================================
+ Coverage 97.58% 97.79% +0.20%
==========================================
Files 18 18
Lines 3147 3127 -20
==========================================
- Hits 3071 3058 -13
+ Misses 76 69 -7 ☔ View full report in Codecov by Sentry. |
|
Is it worth reviewing open PRs to see if there are any that should be merged before this, since this PR may create merge conflicts? |
|
PR #502 certainly would, although it looks relatively easy to resolve. Would it be useful to create a milestone for the next release? That would make it a little easier to see which PRs (that are likely to be merged) might create problems. |
|
I think it should be fine to make a milestone, if that helps manage the PRs. |
|
Ok, I've created the milestone and added a few PRs to it |
|
Should this PR also be on the milestone? |
|
It's a bit annoying that CI is failing here because of a small codecov thing. |
wheeheee
force-pushed
the
misc
branch
2 times, most recently
from
b35a2df to
8549498
Compare
|
I would like to rebase then merge this PR after #502, since it has gotten a little big, and I think it's easier to rebase conflicting PRs one by one on this rather than rebasing this on multiple PRs. Seeking opinions on whether this is a good idea. |
|
I leave the rebasing to you, then take a hopefully final look and merge. How does that sound? |
| @inbounds out[i,j] += abs2(s_fft[i,j])*m1 | ||
| end | ||
| for i in eachindex(out, s_fft) | ||
| @inbounds out[i] = abs2(s_fft[i]) * m1 |
There was a problem hiding this comment.
In this @inbounds helpful? I'd imagine the compiler can eliminate or at lest hoist it out of the loop.
There was a problem hiding this comment.
benchmarked for 1000 x 1000 Float64 and ComplexF64 matrices. doesn't seem to matter.
There was a problem hiding this comment.
initially tried it on v1.10, but removing it in v1.6 has a 30% performance penalty, so I think it should remain, as the eachindex ensures correctness anyway.
I also discovered the performance fix in #516, which feels like just a workaround for vectorization regressions in v1.10 and v1.9, also introduced massive performance regressions on v1.6 for me, so I would like to clarify what DSP.jl's policy on performance is; is only the latest stable version prioritized, or is performance on LTS also important enough to consider multi-versioning?
|
Have you benchmarked whether the |
nextprod with tuple arg in v1.6+
- `eachindex(s, win)` - `ArraySplit` for non 1-based arrays - chained comparisons coz they look pretty - more informative errors
Co-authored-by: Martin Holters <[email protected]>
Co-authored-by: Martin Holters <[email protected]>
Yes, I have them here. Not zeroing doesn't help as much as the inlining, maybe about a 10% difference, so I didn't think to include those benchmarks. julia> versioninfo()
Julia Version 1.10.1
Commit 7790d6f064 (2024-02-13 20:41 UTC)
Build Info:
Official https://julialang.org/ release
Platform Info:
OS: Windows (x86_64-w64-mingw32)
CPU: 8 × 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
WORD_SIZE: 64
LIBM: libopenlibm
LLVM: libLLVM-15.0.7 (ORCJIT, tigerlake)
Threads: 8 default, 0 interactive, 4 GC (on 8 virtual cores)
Environment:
JULIA_CONDAPKG_BACKEND = Null
JULIA_NUM_THREADS = auto |
|
Nice set of updates here! |
DomainError,ArgumentErroretc.)findmin,splice!etc.