Removing the default sampling frequency from create_from_X_y, #143. #256
Conversation
|
uh why did you swap argument order? |
|
you can swap the argument order back and just change the function calls accordingly |
|
Thanks for the feedback, just swapped the order back. |
braindecode/datasets/xy.py
Outdated
| sfreq: common sampling frequency of all trials | ||
| ch_names: array-like | ||
| channel names of the trials | ||
| sfreq: int |
There was a problem hiding this comment.
| sfreq: int | |
| sfreq: float |
braindecode/datasets/xy.py
Outdated
| ch_names: array-like | ||
| channel names of the trials | ||
| sfreq: int | ||
| common sampling frequency of all trials |
There was a problem hiding this comment.
| common sampling frequency of all trials | |
| Sampling frequency of signals. |
braindecode/datasets/xy.py
Outdated
| drop_last_window: bool | ||
| whether or not have a last overlapping window, when | ||
| windows/windows do not equally divide the continuous signal | ||
| ch_names: array-like | ||
| channel names of the trials |
There was a problem hiding this comment.
| channel names of the trials | |
| Names of the channels. |
| @@ -88,12 +88,12 @@ def test_cropped_decoding(): | |||
| # Perform forward pass to determine how many outputs per input | |||
| n_preds_per_input = get_output_shape(model, in_chans, input_window_samples)[2] | |||
|
|
|||
| train_set = create_from_X_y(X[:60], y[:60], | |||
| train_set = create_from_X_y(X[:60], y[:60], 100, | |||
There was a problem hiding this comment.
| train_set = create_from_X_y(X[:60], y[:60], 100, | |
| train_set = create_from_X_y(X[:60], y[:60], sfreq=100, |
| drop_last_window=False, | ||
| window_size_samples=input_window_samples, | ||
| window_stride_samples=n_preds_per_input) | ||
|
|
||
| valid_set = create_from_X_y(X[60:], y[60:], | ||
| valid_set = create_from_X_y(X[60:], y[60:], 100, |
There was a problem hiding this comment.
| valid_set = create_from_X_y(X[60:], y[60:], 100, | |
| valid_set = create_from_X_y(X[60:], y[60:], sfreq=100, |
| @@ -148,12 +148,12 @@ def test_eeg_classifier(): | |||
| out = model(test_input) | |||
| n_preds_per_input = out.cpu().data.numpy().shape[2] | |||
|
|
|||
| train_set = create_from_X_y(X[:48], y[:48], | |||
| train_set = create_from_X_y(X[:48], y[:48], 100, | |||
There was a problem hiding this comment.
| train_set = create_from_X_y(X[:48], y[:48], 100, | |
| train_set = create_from_X_y(X[:48], y[:48], sfreq=100, |
| drop_last_window=False, | ||
| window_size_samples=input_window_samples, | ||
| window_stride_samples=n_preds_per_input) | ||
|
|
||
| valid_set = create_from_X_y(X[48:60], y[48:60], | ||
| valid_set = create_from_X_y(X[48:60], y[48:60], 100, |
There was a problem hiding this comment.
| valid_set = create_from_X_y(X[48:60], y[48:60], 100, | |
| valid_set = create_from_X_y(X[48:60], y[48:60], sfreq=100, |
| @@ -87,7 +87,7 @@ def test_cropped_trial_epoch_scoring(): | |||
| predictions_cases, y_true_cases, expected_accuracies_cases | |||
| ): | |||
| dataset_valid = create_from_X_y( | |||
| np.zeros((4, 1, 10)), np.concatenate(y_true), | |||
| np.zeros((4, 1, 10)), np.concatenate(y_true), 100, | |||
There was a problem hiding this comment.
| np.zeros((4, 1, 10)), np.concatenate(y_true), 100, | |
| np.zeros((4, 1, 10)), np.concatenate(y_true), sfreq=100, |
braindecode/datasets/xy.py
Outdated
| @@ -14,7 +14,7 @@ | |||
|
|
|||
|
|
|||
| def create_from_X_y( | |||
| X, y, drop_last_window, sfreq=None, ch_names=None, window_size_samples=None, | |||
| X, y, sfreq, drop_last_window, ch_names=None, window_size_samples=None, | |||
There was a problem hiding this comment.
this is an API change.
@robintibor deprecation cycle or you can live with it?
|
@agramfort thank you for the corrections. |
|
Can you still add to what's new page? https://github.com/vytjan/braindecode/blob/mandatory_sfreq/docs/whats_new.rst create new subheading of current version API changes (copy heading from below), and add a sentence about this, also add all your names |
Added the PR info to the what's new section (under the Current (0.5.2.dev0) tag, correct?). |
|
Looks good! Merged! Thanks so much! @vytjan @Ann-KathrinKiessner @dcwil @HenrikBons |
Fixes the #143
Also, not sure if it's fine to swap the order of arguments of the create_from_X_y, but as drop_last_window seems to be an optional, it could be better to assign a default value for it maybe?
And finally, since 4 of us (@Ann-KathrinKiessner, @dcwil, @HenrikBons) worked on this small issue for introduction, we'd like to know if (or how) we should add ourselves to the author's list of the respective files.