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.

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

MRG: Make kind=auto #105

Closed
wants to merge 5 commits into from
Closed

Conversation

jasmainak
Copy link
Member

cc @teonbrooks ...

now I need to run for beers though

@@ -570,6 +570,16 @@ def raw_to_bids(subject_id, task, raw_file, output_path, session_id=None,
raise ValueError('raw_file must be an instance of str or BaseRaw, '
'got %s' % type(raw_file))

if 'eeg' in raw and 'meg' in raw:
Copy link
Member

Choose a reason for hiding this comment

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

you could even reduce this logic to if 'meg' in raw since any and meg sensor present makes it meg.

also this will fail because not all meg files have eeg

Copy link
Contributor

Choose a reason for hiding this comment

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

Was going to say "yep, this will fail for some KIT data", then looked at the tests and saw, yep, it failed for KIT data 😆

@teonbrooks
Copy link
Member

closes #73

@codecov-io
Copy link

codecov-io commented Oct 4, 2018

Codecov Report

Merging #105 into master will decrease coverage by 0.32%.
The diff coverage is 76.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
- Coverage   93.14%   92.81%   -0.33%     
==========================================
  Files          14       14              
  Lines         846      863      +17     
==========================================
+ Hits          788      801      +13     
- Misses         58       62       +4
Impacted Files Coverage Δ
mne_bids/tests/test_mne_bids.py 100% <100%> (ø) ⬆️
mne_bids/mne_bids.py 91.86% <100%> (+0.37%) ⬆️
mne_bids/utils.py 89.67% <64.28%> (-1.79%) ⬇️

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 9fe672b...ee0c7a9. Read the comment docs.

@teonbrooks
Copy link
Member

and then add tests. change to MRG when you're ready for final review

@teonbrooks teonbrooks added this to the 0.1.0 release 🎉 milestone Oct 10, 2018
@teonbrooks
Copy link
Member

@jasmainak, any updates?

@jasmainak
Copy link
Member Author

@teonbrooks I think this PR is contingent on this comment and its resolution, hence I'm holding off on this.

@teonbrooks
Copy link
Member

teonbrooks commented Oct 11, 2018

@teonbrooks I think this PR is contingent on this comment and its resolution, hence I'm holding off on this.

these seem independent from each other. the ability to introspect in the raw object doesn't depend on the filename. the code makes logical sense, it just needs test

addendum: if you include @sappelhoff's suggestion for taking the explicit argument of kind within the if statement

@jasmainak jasmainak force-pushed the kind_auto branch 4 times, most recently from 4fb09de to 0bc862f Compare October 12, 2018 06:19
@jasmainak jasmainak changed the title WIP: Make kind=auto MRG: Make kind=auto Oct 12, 2018
@jasmainak
Copy link
Member Author

okay good to go from my end. Fingers crossed for the CIs

mne_bids/utils.py Show resolved Hide resolved
@teonbrooks
Copy link
Member

what's the deal with codecov?

acquisition=acq, task=task, output_path=output_path,
events_data=events_fname, event_id=event_id, overwrite=True)

raw_to_bids(subject_id=subject_id, raw_file=raw_fname, **kwargs)
Copy link
Member

@teonbrooks teonbrooks Oct 14, 2018

Choose a reason for hiding this comment

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

you should include an assertion on the type to make sure it's working properly for a given modality (e.g. 'fif' for meg, '.edf' for eeg

Copy link
Member

@teonbrooks teonbrooks left a comment

Choose a reason for hiding this comment

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

expand tests to validate auto

@jasmainak
Copy link
Member Author

expand tests to validate auto

we already have auto being tested since it's the default ...

@teonbrooks
Copy link
Member

looking through the test_mne_bids.py, you can see that kind is being explicitly called for the EEG files. auto is currently being used for the MEG files bc the default for kind before used 'meg'. one way to show that auto is working correctly would be to change the args of eeg but make sure it is still being validated with the bep006 flag. that would show that auto detected it was eeg and named it accordingly and it worked with the eeg bids validator.

kind : str, one of ('meg', 'eeg', 'ieeg')
The kind of data being converted. Defaults to "meg".
kind : str, one of ('auto', 'meg', 'eeg', 'ieeg')
The kind of data being converted. Defaults to "auto".
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe mention that kind = 'auto' will determine the type of data automatically from the input raw data? Or is it obvious enough from the name?

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.

5 participants