Skip to content

WIP fixes for to_data_frame - #14

Open
drammock wants to merge 3 commits into
mne-tools:masterfrom
drammock:fix-to-data-frame-compat
Open

WIP fixes for to_data_frame#14
drammock wants to merge 3 commits into
mne-tools:masterfrom
drammock:fix-to-data-frame-compat

Conversation

@drammock

@drammock drammock commented Feb 13, 2020

Copy link
Copy Markdown
Member

closes #12

@dengemann this is still WIP but I've managed to get from failing on the first test up to 5 OK, 6 Failed 10 OK, 1 Failed when running devtools::test() when mne-python is on this PR branch. I'll try to work on this off and on, but I'll be gone for half of next week so feel free to push here if you want to keep debugging.

@drammock
drammock marked this pull request as ready for review February 13, 2020 19:04
@drammock

Copy link
Copy Markdown
Member Author

OK @dengemann all existing tests pass, but it doesn't yet test the new functionality (pandas Datetime or Timedelta columns, or keeping time as a float in seconds).

Comment thread R/mne.r
if ("mne.io.base.BaseRaw" %in% class(inst)) {
.args$start <- start
.args$stop <- stop
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How is this behaving with older versions of MNE? It doesn't look very backwards-compatible. Probably we'd need to use inspect a bit more intensely.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

you are definitely right about that. What I have done here is a hacky way to make it compatible with #7206, just to make sure that was possible. In the bigger picture, there are at least 3 changes in MNE-Python #7206 that we need to deal with:

  1. index=None was previously buggy; it was behaving as if it meant index="all". The MNE-R functions and tests are currently written to accommodate this (e.g., they assume for raw instance that time ends up as the row names of the dataframe when index=NULL).
  2. scaling_time is now gone (though perhaps it should have a deprecation cycle?), and there is a new parameter time_format. Default values of the new param should yield the same behavior as default values of the old param, as long as the change in index behavior is accounted for (see point 1 above).
  3. each instance type now has a separate definition of to_data_frame, and start and stop only occur in the signature for Raw.to_data_frame now. This is maybe the hardest point to deal with.

Possible ways forward:

  • let this PR sit until the next stable release of MNE-Python, and then merge this and bump the MNE-R version and bump its dependency to version 0.20 of MNE-Python.
  • write some complicated code that triages based on detected version of MNE-Python
  • write some clever code that handles old and new versions of MNE-Python without trying to detect which version of MNE-Python is available.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would be wise to avoid option 2. It then depends if we can come up with said clever code. I think, in priniciple, using inspect to deal with call signature can get us there but in the end option 1 might be the cleanest thing to do. The dev time gained by that could go in new features and new examples for mne-r.

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.

ENH: better support for pandas time datatypes

2 participants