Fix more warnings - Githubissues

xdf-modules / pyxdf

Python package for working with XDF files

BSD 2-Clause "Simplified" License

37 stars 17 forks source link

Fix more warnings #48

Closed cbrnr closed 5 years ago

cbrnr commented 5 years ago

I've added smoke tests for XDF test files in a local folder. I found two warnings in _clock_sync:

[x] If clock_times (and clock_values) is a list with only one element, time_diff is an empty list and np.median of an empty list raises a warning. I've solved this by handling clock resets only for two or more time stamps. I hope this change doesn't affect anything else I might have overlooked.
[x] With another test file, I get a warning in line 633:
```
RuntimeWarning: divide by zero encountered in true_divide
tmp = np.maximum(0, (1 - (1 + 1 / rho) / np.abs(d)))
```
This is because d contains elements that are exactly 0. I have disabled this warning for this particular line.

cbrnr commented 5 years ago

@tstenner @cboulay ready to merge - please double-check if these changes have any unintended side effects.

cbrnr commented 5 years ago

Also, the test I've added requires a folder with XDF files. We can leave this test in for now, but we should start discussing how we could host test files. For most of the files I have, I will get permission from the original owners to use them in our tests.

cboulay commented 5 years ago

but we should start discussing how we could host test files.

There was some suggestion to use git-lfs, but I don't know how this works with CI. Maybe we should keep that discussion in #38

cbrnr commented 5 years ago

git-lfs should work with CIs even when a repo is cloned before git-lfs is installed. But let's continue this in #38. Meanwhile, I have removed the test again so that this PR can get merged.

tstenner commented 5 years ago

We could put this example file in the repo:

test.xdf.gz

Its size is somewhat reasonable for a repository (554 bytes) and it has two streams, so for a quick check it should suffice.

cbrnr commented 5 years ago

Sure, we could put several small files in the repo, this would be better than nothing. But what is the size limit? Our collection of small test files will still grow so we are only postponing the decision. In MNE-Python, we created a separate repository just for testing data. That way, we don't contaminate our code, we can choose to test specific files (even in CIs where we could use the URLs of specific files instead of cloning the whole repo), or we can download the whole repo for local testing. The downside is that the data still lives in a normal git repo, which tends to be very slow for large binary files.

cbrnr commented 5 years ago

Let's talk about test data in #38. This PR is ready to be merged, could someone please review?

cbrnr commented 5 years ago

Can you just squash and merge?