Skip to content

Fix more warnings#48

Merged
cbrnr merged 7 commits intoxdf-modules:masterfrom
cbrnr:test_load_xdf
Oct 14, 2019
Merged

Fix more warnings#48
cbrnr merged 7 commits intoxdf-modules:masterfrom
cbrnr:test_load_xdf

Conversation

@cbrnr
Copy link
Copy Markdown
Contributor

@cbrnr cbrnr commented Oct 8, 2019

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

  • 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.
  • 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
Copy link
Copy Markdown
Contributor Author

cbrnr commented Oct 9, 2019

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

@cbrnr
Copy link
Copy Markdown
Contributor Author

cbrnr commented Oct 9, 2019

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
Copy link
Copy Markdown
Contributor

cboulay commented Oct 9, 2019

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
Copy link
Copy Markdown
Contributor Author

cbrnr commented Oct 10, 2019

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
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor Author

cbrnr commented Oct 10, 2019

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
Copy link
Copy Markdown
Contributor Author

cbrnr commented Oct 11, 2019

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

Copy link
Copy Markdown
Contributor

@tstenner tstenner left a comment

Choose a reason for hiding this comment

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

See comment nr. 3, apart from that it's a +1 from me

Comment thread pyxdf/pyxdf.py
Comment thread pyxdf/pyxdf.py Outdated
Copy link
Copy Markdown
Contributor

@tstenner tstenner left a comment

Choose a reason for hiding this comment

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

+1, but could you rebase the PR and remove the superceded commits?

@cbrnr
Copy link
Copy Markdown
Contributor Author

cbrnr commented Oct 11, 2019

Can you just squash and merge?

@cbrnr cbrnr merged commit a6c5f84 into xdf-modules:master Oct 14, 2019
@cbrnr cbrnr deleted the test_load_xdf branch October 14, 2019 06:44
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