-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add support for MATLAB files #360
Conversation
Codecov Report
@@ Coverage Diff @@
## master #360 +/- ##
==========================================
- Coverage 94.80% 94.63% -0.17%
==========================================
Files 8 8
Lines 808 839 +31
==========================================
+ Hits 766 794 +28
- Misses 42 45 +3
|
Do we want an integration test for |
I would say no. It should be enough if we unit test how the data is read and what it's transformed to. Once the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minimal changes, but otherwise great job!
We might need to think about a smart way to run integration tests on different types (e.g. matlab, acq, ...) of the same file, but I wouldn't block this PR for that!
setup.cfg
Outdated
@@ -48,6 +50,7 @@ test = | |||
pytest >=5.3 | |||
pytest-cov | |||
%(acq)s | |||
%(mat)s | |||
%(style)s | |||
interfaces = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move interfaces before tests and just change acq and mat in tests to "interfaces"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You sure? I put this here because test were failing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if you add %(mat)s to the interfaces and move it before tests, it will work.
interfaces =
%(acq)s
%(mat)s
test =
pytest >=5.3
pytest-cov
%(interfaces)s
%(style)s
In fact, I will push a little patch to change the way we install phys2bids in CircleCI later, but for the moment, this would be better.
Ah, sorry, I forgot one point: |
Co-authored-by: Stefano Moia <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you @vinferrer ! We're finally adding support for MATLAB 💪🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
🚀 PR was released in |
Closes #25 and closes #292
Proposed Changes
load_mat
function toio.py
Change Type
bugfix
(+0.0.1)minor
(+0.1.0)major
(+1.0.0)refactoring
(no version update)test
(no version update)infrastructure
(no version update)documentation
(no version update)other
Checklist before review