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

Buffer array resample and reference_data #623

Merged
merged 11 commits into from
Feb 6, 2025
Merged

Buffer array resample and reference_data #623

merged 11 commits into from
Feb 6, 2025

Conversation

wheeheee
Copy link
Member

On top of #621

Copy link

codecov bot commented Jan 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.13%. Comparing base (06f8776) to head (6154e85).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #623      +/-   ##
==========================================
+ Coverage   97.97%   98.13%   +0.16%     
==========================================
  Files          19       19              
  Lines        3254     3277      +23     
==========================================
+ Hits         3188     3216      +28     
+ Misses         66       61       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally like DRYing up code by pulling out common code to functions, but those then need a descriptive name to maintain readability. I'm not sure that's always the case here. That aside, this PR increases LOC by 49, 6 of which are a new tests, some are additional usings in the tests (to make those file runable by themselves, I suppose). But that still leaves quite a few additional lines due to NFC code re-organization, where I'm not 100% sure it's a net improvement. Need to let this sink in a bit more.

Meanwhile, some minor remarks.

src/Filters/stream_filt.jl Outdated Show resolved Hide resolved
src/Filters/stream_filt.jl Outdated Show resolved Hide resolved
src/Filters/stream_filt.jl Outdated Show resolved Hide resolved
src/Filters/stream_filt.jl Outdated Show resolved Hide resolved
src/Filters/stream_filt.jl Outdated Show resolved Hide resolved
src/Filters/stream_filt.jl Outdated Show resolved Hide resolved
src/Filters/stream_filt.jl Outdated Show resolved Hide resolved
test/FilterTestHelpers.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
src/Filters/stream_filt.jl Outdated Show resolved Hide resolved
wheeheee and others added 2 commits February 5, 2025 18:07
Co-authored-by: Martin Holters <[email protected]>
Co-authored-by: Martin Holters <[email protected]>
@wheeheee
Copy link
Member Author

wheeheee commented Feb 5, 2025

Huh, macOS and 32-bit failures? Weird.

to `checked_resample_output!`
Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for minor nits.

src/Filters/stream_filt.jl Outdated Show resolved Hide resolved
src/Filters/stream_filt.jl Outdated Show resolved Hide resolved
@wheeheee
Copy link
Member Author

wheeheee commented Feb 6, 2025

If we could use in! the LOAD_PATH thing would be nicer but sadly that's new in 1.11.

@wheeheee wheeheee merged commit 2257010 into master Feb 6, 2025
11 checks passed
@wheeheee wheeheee deleted the buffer_resample branch February 6, 2025 11:14
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 this pull request may close these issues.

2 participants