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

[WIP] Tests #27

Merged
merged 4 commits into from
Mar 5, 2018
Merged

[WIP] Tests #27

merged 4 commits into from
Mar 5, 2018

Conversation

teonbrooks
Copy link
Member

@teonbrooks teonbrooks commented Mar 2, 2018

building from, closes #23, closes #5.

I am adding tests for all four MEG systems since we have test data for them all. Should our tests compare itself to the validator? I think there is supposed to be a python bids validator. Since we are utilizing already tested mne readers for the conversion tool, we shouldn't need to validate them. We need to make sure filenames are correct.

Possible addition: anonymize kwarg in raw_to_bids. We have a function already that does this, we can expose in the conversion step.

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.

Looks like a great start. Thanks @teonbrooks

yes, I think we can use a pure python validator indeed. See here: https://github.com/INCF/pybids/blob/master/bids/grabbids/bids_validator.py

It doesn't have the MEG part though. I could update it once I'm done with the validator on INCF. I merged the other PR so you can focus on the tests in this PR. You will need to rebase :)

@@ -1,2 +1,2 @@
from .bids_meg import raw_to_bids
from .bids_meg import _mkdir_p
from .meg_bids import raw_to_bids
Copy link
Member

Choose a reason for hiding this comment

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

good call from the linguist ;)

event_id=event_id, overwrite=True)


def test_kit():
Copy link
Member

Choose a reason for hiding this comment

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

Missing docstring

@teonbrooks
Copy link
Member Author

I could update it once I'm done with the validator on INCF.

this would be great to have. what I will do for now if make the tests for the four different meg types, and then we can add the validation steps to those tests. I think that would be sufficient.

Included all the MEG systems for tests

TODO: add bids-validation
@teonbrooks
Copy link
Member Author

teonbrooks commented Mar 3, 2018

It doesn't have the MEG part though. I could update it once I'm done with the validator on INCF. I merged the other PR so you can focus on the tests in this PR. You will need to rebase :)

@jasmainak, anything I can do to help with the validator?

- Add default value for unlisted channel types
- Fix support for BTi data
- Fix overwrite for non-FIF files
eog='EOG', ecg='ECG', misc='MISC', ref_meg='REFMEG')
map_desc = dict(grad='Gradiometer', mag='Magnetometer',
map_desc = defaultdict(lambda: 'Other type of channel')
Copy link
Member Author

Choose a reason for hiding this comment

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

The spec has a restricted list of channel types but I wonder if there is a way to preserve some info we extract to put in the description. e.g. resp is a channel type in the BTi data but that info would be lost

Copy link
Member

Choose a reason for hiding this comment

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

hmm ... you can always add it in the description, but it's true that "other" is no longer an available option. Perhaps we should update the MEG draft for that? Although I'd rather not have the draft updated at this point so we can write the software (validator etc.) with what we have

@jasmainak
Copy link
Member

this would be great to have. what I will do for now if make the tests for the four different meg types, and then we can add the validation steps to those tests. I think that would be sufficient.

Yes, that sounds like a great plan!

@jasmainak
Copy link
Member

@jasmainak, anything I can do to help with the validator?

I guess it's mostly a matter of copying the regular expressions and json schemas from the pull request on the INCF website to pybids. If you have time, feel free to make the PR and I'll review it :)

@jasmainak
Copy link
Member

@teonbrooks would you mind adding a travis.yml file so that we can do continuous integration?

@teonbrooks
Copy link
Member Author

yeah, I think we can merge these tests and changes so that we can address @choldgraf PR #28 to generalize the reader. I know that I have modded some code here so that PR will need to rebase and resolve the conflicts

@jasmainak jasmainak merged commit ed12542 into mne-tools:master Mar 5, 2018
@jasmainak
Copy link
Member

Done, thanks @teonbrooks :)

@teonbrooks teonbrooks deleted the tests branch March 5, 2018 22:32
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.

add tests
2 participants