384 deprecate toitoilim and foifoilim for selectdata #385

tensionhead · 2022-12-02T10:21:35Z

Make `selectdata` more similar to `ft_selectdata.m`

toi/toilim and foi/foilim gone in favor of latency and frequency as in ft_selectdata
(only!) toi and toilim were tolerant if the upper or lower boundary were outside of the range of a trial time, leading to inconsistent or at least intransparent results
This is now no longer the case: latency either throws an error if no trial fits, or kicks out trials which are not fitting to the desired time window retaining only the fitting trials
this means, that every latency selection now returns timelocked data , which should be pretty neat to continue with a freqanalysis and so on..
some selection focused tests could be greatly simplified as no toi and foi selections need to be tested anymore

Important

foi/foilim and toi/toilim remain valid parameters for freqanalysis, as also in ft_freqanalysis
foi/foilim remain valid for connectivityanalysys if a mtmfft spectral estimation is to be done on the fly from an AnalogData input
in none of these cases those parameters enter into a selection!

Author Guidelines

Is the change set < 600 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

- also introduced new class attribute `_selectionKeyWords` - to sort out relevant keywords per data class - TODO: repair the myriads of failing tests Changes to be committed: modified: syncopy/datatype/base_data.py modified: syncopy/datatype/continuous_data.py modified: syncopy/datatype/discrete_data.py modified: syncopy/datatype/methods/selectdata.py modified: syncopy/tests/test_selectdata.py

- now on to the rest.. Changes to be committed: modified: syncopy/datatype/base_data.py modified: syncopy/datatype/continuous_data.py modified: syncopy/tests/test_selectdata.py

- previous commit just dealt with `test_general()` Changes to be committed: modified: syncopy/datatype/methods/selectdata.py modified: syncopy/tests/test_selectdata.py

- we don't support unbounded [1, np.inf] interval selections anymore - could be reintroduced if there's a user demand Changes to be committed: modified: syncopy/datatype/base_data.py modified: syncopy/tests/test_continuousdata.py

Changes to be committed: modified: syncopy/datatype/methods/definetrial.py modified: syncopy/tests/test_discretedata.py

- passing plotting, preproc, resampledata Changes to be committed: modified: syncopy/shared/latency.py modified: syncopy/tests/helpers.py modified: syncopy/tests/test_plotting.py modified: syncopy/tests/test_preproc.py modified: syncopy/tests/test_resampledata.py modified: syncopy/tests/test_specest_fooof.py

- connectivity and metadata tests passing

- TODO: the final boss enemy: test_specest Changes to be committed: modified: syncopy/datatype/continuous_data.py modified: syncopy/tests/test_computationalroutine.py modified: syncopy/tests/test_spike_psth.py

- that was a mean bug Changes to be committed: modified: syncopy/shared/computational_routine.py

- victory is ours Changes to be committed: modified: syncopy/tests/test_specest.py

- frontends don't wipe anymore.. Changes to be committed: modified: syncopy/shared/computational_routine.py modified: syncopy/tests/test_specest.py

…ectdata

- also catched missing single frequency selection Changes to be committed: modified: syncopy/datatype/base_data.py modified: syncopy/tests/test_connectivity.py

Changes to be committed: modified: syncopy/plotting/sp_plotting.py modified: syncopy/tests/test_plotting.py

- removed support for np.inf entry Changes to be committed: modified: syncopy/datatype/base_data.py modified: syncopy/tests/test_connectivity.py modified: syncopy/tests/test_selectdata.py

codecov · 2022-12-02T14:57:45Z

Codecov Report

Base: 69.82% // Head: 68.59% // Decreases project coverage by -1.22% ⚠️

Coverage data is based on head (19d03a2) compared to base (8a649c9).
Patch coverage: 83.33% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #385      +/-   ##
==========================================
- Coverage   69.82%   68.59%   -1.23%     
==========================================
  Files          78       78              
  Lines        9460     9403      -57     
  Branches     1931     1920      -11     
==========================================
- Hits         6605     6450     -155     
- Misses       2379     2448      +69     
- Partials      476      505      +29

Impacted Files	Coverage Δ
syncopy/nwanalysis/connectivity_analysis.py	`71.03% <0.00%> (-1.38%)`	⬇️
syncopy/plotting/sp_plotting.py	`68.12% <0.00%> (-10.63%)`	⬇️
syncopy/shared/computational_routine.py	`77.72% <ø> (-0.47%)`	⬇️
syncopy/datatype/methods/selectdata.py	`78.72% <60.00%> (+1.54%)`	⬆️
syncopy/datatype/base_data.py	`78.90% <90.00%> (-3.41%)`	⬇️
syncopy/datatype/continuous_data.py	`81.75% <100.00%> (-3.82%)`	⬇️
syncopy/datatype/discrete_data.py	`78.30% <100.00%> (-5.34%)`	⬇️
syncopy/datatype/methods/definetrial.py	`83.90% <100.00%> (+0.23%)`	⬆️
syncopy/shared/latency.py	`76.71% <100.00%> (ø)`
syncopy/tests/helpers.py	`100.00% <100.00%> (+3.80%)`	⬆️
... and 14 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dfsp-spirit · 2022-12-05T12:14:11Z

Thanks a lot for this change, which brings us closer to fieldtrip!

I have read through all the changes, and I am currently testing it locally.

I just noticed that in our [quickstart guide](https://syncopy.readthedocs.io/en/latest/quickstart/quickstart.html), the first plot command includes a `toilim` selection and therefore fails: `data.singlepanelplot(trials=0, toilim=[0, 0.5])`.

I guess we should double-check that in our doc-sprint branch and fix it there once this is merged. It would be good to complete the quick start guide once ourselves before the new release, just to be sure it is still up-to-date and working with the latest version.

UPDATE: I have fixed this in the quickstart guide already.

dfsp-spirit

Thanks a lot for this change!

syncopy/datatype/methods/selectdata.py

tensionhead · 2022-12-05T16:54:27Z

Thanks for the final fixes and the quick merge 🙂 !

tensionhead added 12 commits December 1, 2022 12:15

WIP: test_selectdata passing

4a19c68

- now on to the rest.. Changes to be committed: modified: syncopy/datatype/base_data.py modified: syncopy/datatype/continuous_data.py modified: syncopy/tests/test_selectdata.py

WIP: ..now all tests within test_selectdata passing

e23c07f

- previous commit just dealt with `test_general()` Changes to be committed: modified: syncopy/datatype/methods/selectdata.py modified: syncopy/tests/test_selectdata.py

WIP: test_continuousdata passes

1a104e0

- we don't support unbounded [1, np.inf] interval selections anymore - could be reintroduced if there's a user demand Changes to be committed: modified: syncopy/datatype/base_data.py modified: syncopy/tests/test_continuousdata.py

WIP: fixup test_discretedata

8370353

Changes to be committed: modified: syncopy/datatype/methods/definetrial.py modified: syncopy/tests/test_discretedata.py

WIP: tests fixup

9ade37d

- connectivity and metadata tests passing

WIP: almost there..

45a9e3e

- TODO: the final boss enemy: test_specest Changes to be committed: modified: syncopy/datatype/continuous_data.py modified: syncopy/tests/test_computationalroutine.py modified: syncopy/tests/test_spike_psth.py

FIX: Avoid in place manipulation of trialdefinition

1c1590d

- that was a mean bug Changes to be committed: modified: syncopy/shared/computational_routine.py

FIX: test_specest passing

7a7ab6c

- victory is ours Changes to be committed: modified: syncopy/tests/test_specest.py

FIX: explicitly wipe in place selection

dbbaa4c

- frontends don't wipe anymore.. Changes to be committed: modified: syncopy/shared/computational_routine.py modified: syncopy/tests/test_specest.py

Merge branch 'dev' into 384-deprecate-toitoilim-and-foifoilim-for-sel…

7bab577

…ectdata

tensionhead linked an issue Dec 2, 2022 that may be closed by this pull request

Deprecate toi/toilim and foi/foilim for selectdata #384

Closed

tensionhead added 3 commits December 2, 2022 14:49

FIX: timedependent coherence test

979ad2d

- also catched missing single frequency selection Changes to be committed: modified: syncopy/datatype/base_data.py modified: syncopy/tests/test_connectivity.py

FIX: CrossSpectral plotting

102fb89

Changes to be committed: modified: syncopy/plotting/sp_plotting.py modified: syncopy/tests/test_plotting.py

FIX: Frequency selection

fb2a7ff

- removed support for np.inf entry Changes to be committed: modified: syncopy/datatype/base_data.py modified: syncopy/tests/test_connectivity.py modified: syncopy/tests/test_selectdata.py

tensionhead marked this pull request as ready for review December 2, 2022 14:57

tensionhead added the FT compat Field Trip compatibility label Dec 2, 2022

Update CHANGELOG.md

74541a4

dfsp-spirit self-assigned this Dec 5, 2022

dfsp-spirit self-requested a review December 5, 2022 13:17

FIX: update toilim with latency in quickstart

19d03a2

dfsp-spirit approved these changes Dec 5, 2022

View reviewed changes

syncopy/datatype/methods/selectdata.py Show resolved Hide resolved

dfsp-spirit merged commit d4a4d75 into dev Dec 5, 2022

tensionhead deleted the 384-deprecate-toitoilim-and-foifoilim-for-selectdata branch December 5, 2022 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

384 deprecate toitoilim and foifoilim for selectdata #385

384 deprecate toitoilim and foifoilim for selectdata #385

tensionhead commented Dec 2, 2022 •

edited by dfsp-spirit

Loading

codecov bot commented Dec 2, 2022 •

edited

Loading

dfsp-spirit commented Dec 5, 2022 •

edited

Loading

dfsp-spirit left a comment

tensionhead commented Dec 5, 2022

384 deprecate toitoilim and foifoilim for selectdata #385

384 deprecate toitoilim and foifoilim for selectdata #385

Conversation

tensionhead commented Dec 2, 2022 • edited by dfsp-spirit Loading

Make selectdata more similar to ft_selectdata.m

Important

Author Guidelines

Reviewer Checklist

codecov bot commented Dec 2, 2022 • edited Loading

Codecov Report

dfsp-spirit commented Dec 5, 2022 • edited Loading

dfsp-spirit left a comment

Choose a reason for hiding this comment

tensionhead commented Dec 5, 2022

tensionhead commented Dec 2, 2022 •

edited by dfsp-spirit

Loading

Make `selectdata` more similar to `ft_selectdata.m`

codecov bot commented Dec 2, 2022 •

edited

Loading

dfsp-spirit commented Dec 5, 2022 •

edited

Loading