Skip to content

Conversation

@agramfort
Copy link
Member

not sure to get it. You expose a bug by adding a new test but the CIs pass. What do I miss?

@mscheltienne
Copy link
Member Author

@agramfort I skipped the CI for now, wanted to push a fix first, probably this afternoon.
Maybe not the smartest thing to do..

@mscheltienne
Copy link
Member Author

mscheltienne commented Oct 25, 2021

I gave it some thought, and IMO, it just doesn't make sense to allow resampling to yield 0 data points and to return an empty instance. I chose to raise an error when it happens. If you disagree and prefer to have resample always return at least one datapoint, then I can change it to return e.g. the first datapoint when new_freq < current_freq / current_n_samples.

I don't see a use case for resampling to a frequency yielding only 1 point, and 0 makes even less sense to me. Please let me know if you can think of a use case!

Previously, resample would return an instance with 0 datapoints, which would then raise errors when e.g. representing.

IN: raw_x_sample.times
array([], dtype=float64)

IN: repr(raw_x_sample)
Traceback (most recent call last):

  File "/var/folders/b1/kx2x00x95ls80zzkct4647sh0000gn/T/ipykernel_11717/1501997967.py", line 1, in <module>
    repr(raw_x_sample)

  File "/Users/scheltie/Documents/git/mne-python/mne/io/base.py", line 1751, in __repr__
    % (name, len(self.ch_names), self.n_times, self.times[-1],

IndexError: index -1 is out of bounds for axis 0 with size 0

@mscheltienne
Copy link
Member Author

I'm guessing <compat / old / py3.7> runs with CUDA, and that in this case, numpy had this issue covered with a ValueError raised. I matched the error type/msg with numpy.

@larsoner larsoner added this to the 0.24 milestone Oct 25, 2021
@larsoner
Copy link
Member

@mscheltienne I pushed a commit to add one more max where it was needed. Feel free to add a line to latest.inc about this bugfix !

@mscheltienne mscheltienne changed the title Resample to 1 sample with sfreq > len(times) fails and return empty times. Make sure resample return at least one sample. Oct 25, 2021
@mscheltienne
Copy link
Member Author

Added changelog entry under your name for this one, as I didn't do much except create the PR 😄
CIs failure looked unrelated. Let's see if it comes back green this time.

@larsoner
Copy link
Member

as I didn't do much except create the PR

And find the problem, and think about a solution, and work through it, and add a test... I think your name should be there, too :)

@larsoner larsoner changed the title Make sure resample return at least one sample. MRG, BUG: Make sure resample return at least one sample Oct 25, 2021
* upstream/main:
  MRG: Change Beer Lambert law default ppf value to 6 (#9843)
  MRG, MAINT: Fix for SciPy pre (#9899)
  TYPO: vrmk -> vmrk (#9896)
  fix dead link [ci skip] (#9898)
@larsoner larsoner merged commit 131c676 into mne-tools:main Oct 25, 2021
@larsoner
Copy link
Member

Thanks @mscheltienne !

@mscheltienne mscheltienne deleted the fix_resample branch October 25, 2021 20:04
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.

3 participants