Skip to content

Conversation

@1himan
Copy link
Contributor

@1himan 1himan commented Jan 14, 2026

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.

@1himan
Copy link
Contributor Author

1himan commented Jan 14, 2026

Do I have to describe the changes in the changelog as well, for such a minor change?

@tsbinns
Copy link
Contributor

tsbinns commented Jan 14, 2026

I would argue the unit conversion behaviour is already described in the scalings parameter description of to_data_frame.

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).

@1himan
Copy link
Contributor Author

1himan commented Jan 17, 2026

I would argue the unit conversion behaviour is already described in the scalings parameter description of to_data_frame.

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 - scalings=None) which is useful for plotting.

@1himan
Copy link
Contributor Author

1himan commented Jan 21, 2026

I don't think, changelog should be updated here, that'd be just noise? @drammock.

Copy link
Member

@drammock drammock left a 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)

Comment on lines +2458 to +2460
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.
Copy link
Member

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...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Default units in raw.to_data_frame()

3 participants