-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Improve docs for raw.to_data_frame #13590
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
base: main
Are you sure you want to change the base?
Improve docs for raw.to_data_frame #13590
Conversation
|
Do I have to describe the changes in the changelog as well, for such a minor change? |
|
I would argue the unit conversion behaviour is already described in the Also, the note has it the wrong way around: data is stored internally in standard (SI) units (e.g., V for EEG data), but what the method returns is the data scaled into a more appropriate range (e.g., uV for EEG data). |
You're right. after reading more on this, I found out that this method doesn't do any "conversions" on data at all, rather it returns the data(picked channels) with scaling factors applied(default behaviour - |
|
I don't think, changelog should be updated here, that'd be just noise? @drammock. |
drammock
left a comment
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.
besides the 1 comment below, I'd agree that docstring fixes don't necessarily need a changelog entry (sometimes we do, if the docstring changed to match the behavior, and they were originally quite different... but here I think it's more of a refinement rather than a big change so I'm OK with no changelog entry)
| The default purpose of this method is to return data(picked channels) with | ||
| scalings applied which is useful for plotting. See :term:`data channels` and | ||
| :term:`non-data channels` for the default scaling factors applied. |
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 like that this adds a cross-ref to :term:``data channels. But I think it makes more sense to improve the param description of scalings to do that, rather than adding a new Notes section --- partly because you've helped me realize that the scalings param description is incomplete/inaccurate (only mentions 3 channel types), but also because I don't presume to tell people what the purpose of the method is. People might want their data in a DataFrame for lots of different reasons (plots that we don't natively provide; running statistical models; data sharing w/ non-Python-programmer colleagues...)
Reference issue (if any)
Fixes #12701
What does this implement/fix?
Additional information
Just a minor improvement for more clarity in the documentation.
An added notes section under raw.to_data_frame(), explicitly explaining the default behaviour of that api, which is to convert the data to SI units before returning it by default.