-
Notifications
You must be signed in to change notification settings - Fork 7
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
Release 0.7.0 #94
Release 0.7.0 #94
Conversation
Update develop branch with v0.6.2 changes
Add mffdiff
Update from 0.6.3
MAINT: Use FileNotFound rather than ValueError
feat: add ability to read bad channels from infoN.xml files
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.
Just a couple minor things, then should be good to go.
@@ -289,6 +289,19 @@ def _parse_calibration(self, cali): | |||
} | |||
return ans | |||
|
|||
@cached_property | |||
def channels(self) -> List[Dict[str, Any]]: | |||
"""returns list of good and bad channels""" |
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.
As it is written now, it seems like this function only finds bad channels listed in the data info XML. Maybe rename this method to bad_channels
? From the changelog, it looks like this is the intended funcitonality.
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.
Hm, just looking at the code, it seems like it would fetch all channels, not just bad ones. Can you explain?
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.
>>> import mffpy
>>> dinfo = mffpy.XML.from_file('examples/example_1.mff/info1.xml')
>>> dinfo.channels
[{'channels': [31, 47, 65, 79], 'exclusion': 'badChannels'}]
The way it is written, DataInfo.channels
will parse the text in this channels
tag, which looks something like this
<channels exclusion="badChannels">31 47 65 79</channels>
This is what the MFF API docs from EGI say about this tag:
This is a comma separated list of channels that have some "attribute" to take into account
- exclusion
- goodChannels
- badChannels
That's not super descriptive, but in my experience, I have only ever seen bad channels listed in this tag. I am guessing Damian added this method to get the bad channels for showing in sourcerer? Can you comment @damian5710?
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.
I haven't seen the list being comma separated in my MFF examples. The property is called .channels
b/c the XML tag is called <channels>...
.
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.
I suppose the docstring is technically true, but to me it sounds like it's going to return a list of all channels. Maybe the docstring could just be
"""return contents of <channels> element"""
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.
I haven't seen the list being comma separated in my MFF examples. The property is called
.channels
b/c the XML tag is called<channels>...
.
Yeah, I don't super trust those EGI docs. They're old.
No description provided.