Skip to content

Conversation

@larsoner
Copy link
Member

Implements the first part of #9406 (comment)

Having implemented it, it seems like a really nice separation of functionality actually. mne/epochs.py and mne/io/base.py are cleaner, tests are in mne/export/tests, and mne/export/_eeglab.py can have un-nested imports for its implementation, which is nice.



@verbose
def export_raw(fname, raw, fmt='auto', verbose=None):
Copy link
Member

Choose a reason for hiding this comment

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

should we have both BaseRaw.export and Epochs.export? it seems quite redundant here no?

so maybe?

Suggested change
def export_raw(fname, raw, fmt='auto', verbose=None):
def _export_raw(fname, raw, fmt='auto', verbose=None):

Copy link
Member Author

Choose a reason for hiding this comment

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

My idea is to expose a uniform interface mne.export.export_* to people for all exporting functions (raw, epochs, and evoked), as it's nice to expose all exporting methods there. It is redundant with mne.Epochs.export(...), but so are mne.Epochs.plot / mne.viz.plot_epochs, mne.Covariance.save / mne.write_cov, etc. -- I think of this as a similar method-to-function mapping.

Copy link
Member Author

Choose a reason for hiding this comment

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

(I can live without it though if you're not convinced)

Copy link
Member

Choose a reason for hiding this comment

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

@drammock you decide :)

Copy link
Member

Choose a reason for hiding this comment

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

I don't object to the function/method redundancy.

Question about Evokeds: the class method on Evoked would pass [self] to the mne.export.export_evokeds right? So the function can handle multiple instances, but the class method only one instance? That's how I would expect it to work anyway.

I don't see the class-method versions in this changeset? Is that in a separate PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see the class-method versions in this changeset? Is that in a separate PR?

You mean Epochs.export(...) and Raw.export(...)? Those are already in main. Evoked.export will be part of #9406

the class method on Evoked would pass [self] to the mne.export.export_evokeds right...

Yep, that's the plan

Copy link
Member

Choose a reason for hiding this comment

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

I don't see the class-method versions in this changeset? Is that in a separate PR?

You mean Epochs.export(...) and Raw.export(...)? Those are already in main.

Never mind, I just wasn't reading the diff carefully enough. I see now the nested from .export import export_epochs

@agramfort
Copy link
Member

agramfort commented May 26, 2021 via email

@larsoner larsoner merged commit abfe37b into mne-tools:main May 26, 2021
@larsoner larsoner deleted the export branch May 26, 2021 22:03
larsoner added a commit to larsoner/mne-python that referenced this pull request Jun 2, 2021
* upstream/main:
  [MRG] change utils.logger.warning -> utils.warn (mne-tools#9434)
  FIX : rank computation from info now uses SSS proc history if only grad or mag are present (mne-tools#9435)
  MRG: Enable interpolation for all fNIRS types (mne-tools#9431)
  FIX: brain save_movie (mne-tools#9426)
  ENH: Add mne.export (mne-tools#9427)
  ENH: Test more on pre [skip circle] (mne-tools#9423)
  MRG, ENH: Speed up brain test (mne-tools#9422)
  MAINT: Update URL [ci skip]
  MNT: Reduce number of calls to _update (mne-tools#9407)
  MRG: Tutorial improvements (mne-tools#9416)
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.

3 participants