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.

Sign up for GitHub

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

Jump to bottom

Add Resampling #289

Merged
merged 13 commits into from
Jun 2, 2022
Merged

Add Resampling #289

merged 13 commits into from
Jun 2, 2022

Conversation

tensionhead
Copy link
Contributor

Things added:

  • backend resampling interfacing SciPy's resampling_poly
  • backend tests
  • frontend integration in /preproc/resampledata.py

Still todo

  • frontend tests

Author Guidelines

  • Is the change set < 400 lines?
  • Was the code checked for memory leaks/performance bottlenecks?
  • Is the code running locally and on the ESI cluster?
  • Is the code running on all supported platforms?

Reviewer Checklist

  • Are testing routines present?
  • Do parallel loops have a set length and correct termination conditions?
  • Do objects in the global package namespace perform proper parsing of their input?
  • Do code-blocks provide novel functionality, i.e., no re-factoring using builtin/external packages possible?
  • Code layout
    • Is the code PEP8 compliant?
    • Does the code adhere to agreed-upon naming conventions?
    • Are keywords clearly named and easy to understand?
    • No commented-out code?
  • Are all docstrings complete and accurate?

tensionhead added 11 commits May 24, 2022 11:42
@tensionhead
CHF: Extended doc-string and logging
bb5d6ad 
- keep the original sampling rate in the .log

On branch resampling
Changes to be committed:
	modified:   syncopy/preproc/resampledata.py
@tensionhead
NEW: Basic interface to scipy's resample implementation
e135549 
- this is pure (yet thin) backend atm, if SciPy's implementation is
good enough for our needs this little code might move directly into a cF

On branch resampling
Changes to be committed:
	modified:   syncopy/preproc/firws.py
	new file:   syncopy/preproc/resampling.py
@tensionhead
CHG: Add little helper to retrieve dpss settings from tapsmofrq
c286887 
- also in the backend it is much nicer to work with tapsmofrq
directly, hence added dedicated little function returning the needed
scipy.signal.windows.dpss parameters is nice to have

On branch resampling
Changes to be committed:
	modified:   syncopy/shared/input_processors.py
	modified:   syncopy/specest/mtmfft.py
@tensionhead
NEW: Backend tests for resampling and downsampling
447a92b 
- both trivial downsampling (+FIR) and non-trivial resampling get
tested

On branch resampling
Changes to be committed:
	modified:   syncopy/preproc/compRoutines.py
	modified:   syncopy/preproc/resampling.py
	modified:   syncopy/tests/backend/test_conn.py
	new file:   syncopy/tests/backend/test_resampling.py
@tensionhead
CHG: Relax resampling tests and code formatting
40d4679 
- \pm 5% of expected gain as tolerance for the tests

On branch resampling
Your branch is up to date with 'origin/resampling'.

Changes to be committed:
	modified:   syncopy/preproc/firws.py
	modified:   syncopy/preproc/preprocessing.py
	modified:   syncopy/preproc/resampledata.py
	modified:   syncopy/preproc/resampling.py
	modified:   syncopy/tests/backend/test_resampling.py
@tensionhead
NEW: Test scipy resampling with homegrown firws
f5b7595 
- this shows superior spectral response, yet needs a normalization
tweak :/
- also fixed sampling rate mixup.. (rs_fs, ds_fs)

On branch resampling
Your branch is up to date with 'origin/resampling'.

Changes to be committed:
	modified:   syncopy/tests/backend/test_resampling.py
@tensionhead
CHG: Tighten resample_poly + own firws test criteria
eb2c41a 
- this strategy produces very good results, hence we can be more
thorough with testing

On branch resampling
Your branch is up to date with 'origin/resampling'.

Changes to be committed:
	modified:   syncopy/tests/backend/test_resampling.py
@tensionhead
NEW: Resampling frontend integration
f153b61 
- homegrown firws + SciPy's resample_poly for resampling
- no default anti-aliasing for downsampling as in FT

On branch resampling
Your branch is up to date with 'origin/resampling'.

Changes to be committed:
	modified:   syncopy/preproc/compRoutines.py
	modified:   syncopy/preproc/preprocessing.py
	modified:   syncopy/preproc/resampledata.py
@tensionhead
FIX: Correct FIR filters for resampling
b2ffbdc 
- corrected typo for SciPy's default kaiser fir window (5. and not
.5), the ringing is gone  yet it is still a quite slow roll off
compared with our homegrown firws which remains the frontend default
- when designing a fir for an up-fir-down operation, took the
effective sampling rate during filtering into accout
- refactored backend and cF

On branch resampling
Your branch is up to date with 'origin/resampling'.

Changes to be committed:
	modified:   syncopy/preproc/compRoutines.py
	modified:   syncopy/preproc/resampling.py
	modified:   syncopy/tests/backend/test_resampling.py
@tensionhead
Merge branch 'dev' into resampling
dae75ec
@tensionhead
FIX: Propagate new one-sided tapsmofrq
6d20545 
- for FT compat

On branch resampling
Your branch is up to date with 'origin/resampling'.

Changes to be committed:
	modified:   syncopy/specest/mtmfft.py
@tensionhead tensionhead requested a review from pantaray June 1, 2022 15:59
@tensionhead tensionhead added the Feature Request A concrete request (as detailed as possible) to either add or change functionality. label Jun 1, 2022
@codecov
Copy link

codecov bot commented Jun 1, 2022 •
edited
Loading

Codecov Report

Merging #289 (9646436) into dev (4164b45) will increase coverage by 0.29%.
The diff coverage is 62.89%.

@@            Coverage Diff             @@
##              dev     #289      +/-   ##
==========================================
+ Coverage   68.09%   68.39%   +0.29%     
==========================================
  Files          67       69       +2     
  Lines        7667     7809     +142     
  Branches     1588     1614      +26     
==========================================
+ Hits         5221     5341     +120     
- Misses       2087     2110      +23     
+ Partials      359      358       -1
Impacted Files Coverage Δ
syncopy/tests/backend/test_conn.py 100.00% <ø> (ø)
syncopy/preproc/resampledata.py 19.40% <11.76%> (-21.23%) ⬇️
syncopy/preproc/compRoutines.py 67.06% <25.71%> (-7.94%) ⬇️
syncopy/shared/input_processors.py 52.94% <66.66%> (+1.01%) ⬆️
syncopy/preproc/preprocessing.py 76.03% <84.21%> (ø)
syncopy/preproc/resampling.py 88.46% <88.46%> (ø)
syncopy/preproc/firws.py 82.81% <90.90%> (ø)
syncopy/specest/mtmfft.py 100.00% <100.00%> (ø)
syncopy/tests/backend/test_resampling.py 100.00% <100.00%> (ø)
syncopy/shared/computational_routine.py 78.18% <0.00%> (+0.51%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4164b45...9646436. Read the comment docs.

@pantaray pantaray self-assigned this Jun 2, 2022
pantaray
pantaray reviewed Jun 2, 2022
View reviewed changes
Copy link
Member

@pantaray pantaray left a comment

Choose a reason for hiding this comment

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

The backend interface looks very clean and robust. I anticipate some problems with the frontend, though (see comments). However, without tests, it's hard to say. One side-note: the current version of local_spy.py (here as well as in master) crashes with a TypeError: only integer scalar arrays can be converted to a scalar index raised in synth_data.AR2_network. Not sure if that's just a hiccup in the script or an actual bug.

syncopy/preproc/compRoutines.py Show resolved Hide resolved
syncopy/preproc/compRoutines.py Show resolved Hide resolved
syncopy/preproc/compRoutines.py

# we need to re-calculate the resampling factor
factor = self.cfg['new_samplerate'] / data.samplerate
trafo_trl = lambda trldef: np.ceil(trldef * factor)
Copy link
Member

Choose a reason for hiding this comment

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

As above, I think this might break things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so here I did my own on the fly tests and it looked fine.. again let's see in the tests?!

syncopy/preproc/resampledata.py Show resolved Hide resolved
syncopy/preproc/resampledata.py Show resolved Hide resolved
syncopy/preproc/resampling.py Show resolved Hide resolved
pantaray and others added 2 commits June 2, 2022 12:16
@pantaray
CHG: Minor corrections
3d2d503 
- removed superfluous imports and fixed some docstring typos

 On branch resampling
 Changes to be committed:
	modified:   syncopy/preproc/resampledata.py
	modified:   syncopy/preproc/resampling.py
	modified:   syncopy/tests/backend/test_resampling.py
@tensionhead
FIX: Repaired local_spy.py
9646436 
- the ad-hoc collect_trials decorator accepts only *args for the
wrapper (nTrials, samplerate) but NOT for the wrapped trial (np.array)
producing function.. if that gives too much problems then maybe we
have to think of sth else

Changes to be committed:
	modified:   syncopy/tests/local_spy.py
@tensionhead
Copy link
Contributor Author

The backend interface looks very clean and robust. I anticipate some problems with the frontend, though (see comments). However, without tests, it's hard to say. One side-note: the current version of local_spy.py (here as well as in master) crashes with a TypeError: only integer scalar arrays can be converted to a scalar index raised in synth_data.AR2_network. Not sure if that's just a hiccup in the script or an actual bug.

I fixed it and wrote a little comment in the commit msg!

@pantaray
Copy link
Member

pantaray commented Jun 2, 2022

The collect_trials decorator is quite clever! The trl/factor stuff aside, I have no objections. Ready to merge :)

@pantaray pantaray merged commit b4b96b6 into dev Jun 2, 2022
@tensionhead tensionhead mentioned this pull request Jun 3, 2022
Closed
@tensionhead tensionhead deleted the resampling branch June 3, 2022 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pantaray pantaray pantaray left review comments

Assignees

@pantaray pantaray

Labels
Feature Request A concrete request (as detailed as possible) to either add or change functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tensionhead @pantaray