Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[MRG] fix channel units for EEG and iEEG #125

Merged
merged 6 commits into from
Nov 13, 2018
Merged

[MRG] fix channel units for EEG and iEEG #125

merged 6 commits into from
Nov 13, 2018

Conversation

sappelhoff
Copy link
Member

@sappelhoff sappelhoff commented Nov 2, 2018 •
edited
Loading

closes #81

Using MNE-Python Master (or soon 0.17), I will be able to fix this for BrainVision and EDF (the primary formats both for EEG and iEEG) ... and probably also for BDF and EEGLAB.

I will not be able to fix it for

  • Neuroscan CNT
  • Any MEG system (no knowledge whatsoever and unfortunately not a lot of time to invest)

@jasmainak jasmainak force-pushed the master branch 3 times, most recently from 1c013b9 to c43822e Compare November 5, 2018 06:35
@jasmainak
Copy link
Member

PR needs rebase ...

@jasmainak jasmainak added this to the 0.2 milestone Nov 6, 2018
@sappelhoff sappelhoff closed this Nov 9, 2018
@sappelhoff sappelhoff deleted the channel_units branch November 9, 2018 22:05
@sappelhoff sappelhoff restored the channel_units branch November 9, 2018 22:07
@sappelhoff sappelhoff reopened this Nov 9, 2018
@sappelhoff
Copy link
Member Author

so much for my rebase 🤣

screenshot from 2018-11-09 23-16-32

@jasmainak
Copy link
Member

haha :)

@sappelhoff let me know when you're ready here

@sappelhoff sappelhoff changed the title [WIP] fix channel units for EEG and iEEG [MRG] fix channel units for EEG and iEEG Nov 11, 2018
jasmainak
jasmainak reviewed Nov 11, 2018
View reviewed changes
Copy link
Member

@jasmainak jasmainak left a comment

Choose a reason for hiding this comment

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

also, you have the CIs failing

mne_bids/tests/test_mne_bids.py Outdated Show resolved Hide resolved
mne_bids/tests/test_mne_bids.py Outdated Show resolved Hide resolved
mne_bids/tests/test_mne_bids.py Outdated Show resolved Hide resolved
@jasmainak
Copy link
Member

@sappelhoff rebasing against master will help me verify if the CIs are good on this PR. Otherwise, LGTM. I can merge as soon as I see green CIs

@sappelhoff sappelhoff force-pushed the channel_units branch 3 times, most recently from 0f0aec3 to 40225b7 Compare November 12, 2018 11:15
@codecov-io
Copy link

codecov-io commented Nov 12, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@629f5eb). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #125   +/-   ##
=========================================
  Coverage          ?   93.44%           
=========================================
  Files             ?       14           
  Lines             ?      930           
  Branches          ?        0           
=========================================
  Hits              ?      869           
  Misses            ?       61           
  Partials          ?        0
Impacted Files Coverage Δ
mne_bids/tests/test_mne_bids.py 100% <100%> (ø)
mne_bids/mne_bids.py 92.96% <100%> (ø)
mne_bids/utils.py 91.37% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 629f5eb...a7de0b0. Read the comment docs.

@sappelhoff
Copy link
Member Author

@jasmainak ready for merge

@jasmainak
Copy link
Member

You need to rebase. There are conflicts ...

@sappelhoff
Copy link
Member Author

are we seeing the same thing?
image

@jasmainak
Copy link
Member

haha nopes. Try refreshing your page or loading in a different browser.

@sappelhoff
Copy link
Member Author

haha nopes. Try refreshing your page or loading in a different browser.

you are right, there was something off, but it should be rebased and fine now.

Weird though, that Github would show me that everything's fine even before ... perhaps some browser extensions that prevent proper functioning?

jasmainak
jasmainak reviewed Nov 13, 2018
View reviewed changes
CONTRIBUTING.md
@@ -23,7 +23,7 @@ pypi. Update your installation as follows.

Then, install the following python packages:

$ pip install flake8 pytest pytest-cov
$ pip install flake8 pytest pytest-cov coverage
Copy link
Member

Choose a reason for hiding this comment

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

why do we need coverage ?

Copy link
Member Author

Choose a reason for hiding this comment

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

because pytest-cov relies on it and we need pytest-cov for our coverage reports

@jasmainak
Copy link
Member

Great ! I'll merge once the CIs are happy.

Refreshing browser and restarting computer are the best debugging tools ever. Only next to a good night's sleep ;-)

@sappelhoff
Copy link
Member Author

seems like we have to fix #131 before ...

@jasmainak jasmainak merged commit 828778d into mne-tools:master Nov 13, 2018
@sappelhoff sappelhoff deleted the channel_units branch November 13, 2018 16:52
@jasmainak
Copy link
Member

I'm merging this to move on so we can focus on #131.

But it kind of sucks that the validator fails without warning and no deprecation cycle. Maybe something to discuss with @chrisfilo ...

@sappelhoff
Copy link
Member Author

yes, also thinking about all the examples on https://github.com/bids-standard/bids-examples.

They are not failing yet, because an old version of the validator is pinned at the CI. But it's something to fix soon, moving on to #131 now

@jasmainak
Copy link
Member

Oh I see. We should also not use the dev version. It's not supposed to be stable ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasmainak jasmainak jasmainak left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.2
Development

Successfully merging this pull request may close these issues.

[BUG] _channels_tsv writes default unit for channel type
3 participants
@sappelhoff @jasmainak @codecov-io