search

JuliaDSP / DSP.jl

Filter design, periodograms, window functions, and other digital signal processing functionality
https://docs.juliadsp.org/dev/
Other
374 stars 108 forks source link

general cleanups #536

Closed wheeheee closed 4 months ago

wheeheee commented 5 months ago
codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 99.62825% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 97.79%. Comparing base (10a7c1e) to head (dc9a4bb).

Files Patch % Lines
src/periodograms.jl 98.46% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ViralBShah commented 5 months ago

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?

wheeheee commented 5 months ago

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. Or I could just rebase and fix nearer to the end?

ViralBShah commented 5 months ago

I think it should be fine to make a milestone, if that helps manage the PRs.

wheeheee commented 5 months ago

Ok, I've created the milestone and added a few PRs to it

ViralBShah commented 5 months ago

Should this PR also be on the milestone?

ViralBShah commented 5 months ago

It's a bit annoying that CI is failing here because of a small codecov thing.

wheeheee commented 4 months ago

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. Of course, I'll also take care of rebasing those conflicting PRs.

Seeking opinions on whether this is a good idea.

martinholters commented 4 months ago

I leave the rebasing to you, then take a hopefully final look and merge. How does that sound?

martinholters commented 4 months ago

Have you benchmarked whether the @inlines you have added are actually beneficial?

wheeheee commented 4 months ago

Have you benchmarked whether the @inlines you have added are actually beneficial?

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. Even with the inlining, those functions don't really matter much in the grand scheme of things (for example in digitalfilter, bilinear dominates), maybe only if one makes heavy use of Butterworth and friends, or maybe a lot of Elliptic. As always, the usual disclaimers apply about platform, hardware, etc., so I'll post mine here:

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
ViralBShah commented 4 months ago

Nice set of updates here!